Use tighter typing in editing: EditingStyle

Use tighter typing in editing: EditingStyle

Use tighter typing in editing to increase code readability. This CL focuses
on EditingStyle.

R=leviw@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179251

[eseidel, 2014-07-26 21:19:15]

Unclear if you want the HTMLElement changes.

https://codereview.chromium.org/423673002/diff/1/Source/core/editing/EditingS...
File Source/core/editing/EditingStyle.h (right):

https://codereview.chromium.org/423673002/diff/1/Source/core/editing/EditingS...
Source/core/editing/EditingStyle.h:161: bool
conflictsWithInlineStyleOfElement(HTMLElement*, EditingStyle* extractedStyle,
Vector<CSSPropertyID>* conflictingProperties) const;
Again, this clearly assumes non-SVG content?  It's possible to have SVG content
inside HTML as well...

[Inactive, 2014-07-26 21:41:40]

On 2014/07/26 21:19:15, eseidel wrote:
> Unclear if you want the HTMLElement changes.
> 
>
https://codereview.chromium.org/423673002/diff/1/Source/core/editing/EditingS...
> File Source/core/editing/EditingStyle.h (right):
> 
>
https://codereview.chromium.org/423673002/diff/1/Source/core/editing/EditingS...
> Source/core/editing/EditingStyle.h:161: bool
> conflictsWithInlineStyleOfElement(HTMLElement*, EditingStyle* extractedStyle,
> Vector<CSSPropertyID>* conflictingProperties) const;
> Again, this clearly assumes non-SVG content?  It's possible to have SVG
content
> inside HTML as well...

Do you mean that we might want to stick with Element instead of HTMLElement here
in case we want to add support for SVGElement in the future? To be clear, my
"stricter typing" CLs do not alter the behavior in any way (at least not on
purpose). If I use HTMLElement, it is because all the callers have HTMLElement
or the method only supports HTMLElements at the moment.

For e.g., in the case of conflictsWithInlineStyleOfElement(), all the current
callers (2 of them, in ApplyStyleCommand.cpp) pass an HTMLElement.

[yosin_UTC9, 2014-07-29 04:30:14]

LGTM


For SVG editing, it seems we can edit SVGTextNode in current implementation. I
think "real" SVG editing should be figure/shape base editor, e.g. Adobe
Illustrator.

[Inactive, 2014-07-29 18:57:39]

On 2014/07/29 04:30:14, Yosi_UTC9 wrote:
> LGTM

Thanks. Levi, can you please take a look as well?

[leviw_travelin_and_unemployed, 2014-07-29 21:59:18]

From what I can tell, we have only the most basic SVG editing tests making sure
things don't explode. I'm not opposed strengthening this typing if it's both the
only way it's currently used, and (more importantly) the only way we currently
test it.

This code ultimately looks okay to me, but I'd like a comment from PDR.

https://codereview.chromium.org/423673002/diff/1/Source/core/editing/EditingS...
File Source/core/editing/EditingStyle.h (right):

https://codereview.chromium.org/423673002/diff/1/Source/core/editing/EditingS...
Source/core/editing/EditingStyle.h:161: bool
conflictsWithInlineStyleOfElement(HTMLElement*, EditingStyle* extractedStyle,
Vector<CSSPropertyID>* conflictingProperties) const;
On 2014/07/26 21:19:15, eseidel wrote:
> Again, this clearly assumes non-SVG content?  It's possible to have SVG
content
> inside HTML as well...

From what I can tell, the only tests covering editing SVG are to test for
assertions. I'm not opposed strengthening this typing if it's both the only way
it's currently used, and (more importantly) the only way we test it for the time
being.

[Inactive, 2014-07-29 22:11:22]

On 2014/07/29 21:59:18, leviw wrote:
> From what I can tell, we have only the most basic SVG editing tests making
sure
> things don't explode. I'm not opposed strengthening this typing if it's both
the
> only way it's currently used, and (more importantly) the only way we currently
> test it.

Yes, this CL does not change the behavior. I used HTMLElement type only in cases
where is was already implied.

[pdr., 2014-07-29 22:33:14]

SVG LGTM

https://codereview.chromium.org/423673002/diff/1/Source/core/editing/EditingS...
File Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/423673002/diff/1/Source/core/editing/EditingS...
Source/core/editing/EditingStyle.cpp:1046: if (!element->isHTMLElement())
Nit: Unlike the other cases, this seems like it could make a future refactoring
more difficult since it's not just tightening types.

[Inactive, 2014-07-29 23:14:59]

https://codereview.chromium.org/423673002/diff/1/Source/core/editing/EditingS...
File Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/423673002/diff/1/Source/core/editing/EditingS...
Source/core/editing/EditingStyle.cpp:1046: if (!element->isHTMLElement())
On 2014/07/29 22:33:14, pdr wrote:
> Nit: Unlike the other cases, this seems like it could make a future
refactoring
> more difficult since it's not just tightening types.

Ok, I'll drop this part of this change.

[Inactive, 2014-07-29 23:34:33]

https://codereview.chromium.org/423673002/diff/1/Source/core/editing/EditingS...
File Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/423673002/diff/1/Source/core/editing/EditingS...
Source/core/editing/EditingStyle.cpp:1046: if (!element->isHTMLElement())
On 2014/07/29 23:14:59, Chris Dumez wrote:
> On 2014/07/29 22:33:14, pdr wrote:
> > Nit: Unlike the other cases, this seems like it could make a future
> refactoring
> > more difficult since it's not just tightening types.
> 
> Ok, I'll drop this part of this change.

Done.

[pdr., 2014-07-30 00:57:11]

I think this is good to go.

@eseidel/leviw, did you have any further concerns?

[leviw_travelin_and_unemployed, 2014-07-30 20:19:00]

Nope! LGTM.

[Inactive, 2014-07-30 20:19:44]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-07-30 20:20:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/423673002/20001

[commit-bot: I haz the power, 2014-07-30 20:28:14]

Change committed as 179251