Handle null baseStyle in getPropertiesNotIn

Handle null baseStyle in getPropertiesNotIn

Rather than ASSERT baseStyle, we should just return
early as there are no properties to exclude.

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

[rwlbuis, 2015-02-27 00:02:34]

rob.buis@samsung.com changed reviewers:
+ yosin@google.com

[rwlbuis, 2015-02-27 00:02:35]

PTAL

[rwlbuis, 2015-02-27 03:30:53]

rob.buis@samsung.com changed reviewers:
+ yosin@chromium.org
- yosin@google.com

[rwlbuis, 2015-02-27 03:30:53]

Oops, use correct email address :)

[rwlbuis, 2015-03-02 21:10:10]

On 2015/02/27 03:30:53, rwlbuis wrote:
> Oops, use correct email address :)

ping

[yosin_UTC9, 2015-03-10 05:27:07]

https://codereview.chromium.org/960353002/diff/20001/LayoutTests/editing/styl...
File LayoutTests/editing/style/justify-left-crash.html (right):

https://codereview.chromium.org/960353002/diff/20001/LayoutTests/editing/styl...
LayoutTests/editing/style/justify-left-crash.html:10:
document.execCommand("selectAll", true , null);
nit: We don't need to have |true| and |null|. "SelectAll" command doesn't take
parameters.

https://codereview.chromium.org/960353002/diff/20001/LayoutTests/editing/styl...
LayoutTests/editing/style/justify-left-crash.html:11:
document.execCommand("italic", false , null);
nit: We don't need to have |false| and |null|. These are default parameter
values.

https://codereview.chromium.org/960353002/diff/20001/LayoutTests/editing/styl...
LayoutTests/editing/style/justify-left-crash.html:12:
document.execCommand("justifyLeft", false , null);
nit: We don't need to have |false| and |null|. These are default parameter
values.

https://codereview.chromium.org/960353002/diff/20001/LayoutTests/editing/styl...
LayoutTests/editing/style/justify-left-crash.html:13: </script>
Could you add output of this test to avoid empty expectation?
We usually print "PASS if Blink doesn't crash." or so.

https://codereview.chromium.org/960353002/diff/20001/Source/core/editing/Edit...
File Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/960353002/diff/20001/Source/core/editing/Edit...
Source/core/editing/EditingStyle.cpp:1431:
RefPtrWillBeRawPtr<CSSComputedStyleDeclaration> computedStyle =
position.computedStyle();
Where |position| does point? Does |point| point to |Document|?
Since |Position::computedStyle()| returns computed style of element, it should
not be null.
Call sites, in this case |ApplyStyleCommand::applyBlockStyle|, should not try to
extract style for |Position| which doesn't have style.

[rwlbuis, 2015-03-10 23:44:21]

PTAL, I now exit early if the position is not for an Element.

https://codereview.chromium.org/960353002/diff/20001/LayoutTests/editing/styl...
File LayoutTests/editing/style/justify-left-crash.html (right):

https://codereview.chromium.org/960353002/diff/20001/LayoutTests/editing/styl...
LayoutTests/editing/style/justify-left-crash.html:10:
document.execCommand("selectAll", true , null);
On 2015/03/10 05:27:07, Yosi_UTC9 wrote:
> nit: We don't need to have |true| and |null|. "SelectAll" command doesn't take
> parameters.

Done.

https://codereview.chromium.org/960353002/diff/20001/LayoutTests/editing/styl...
LayoutTests/editing/style/justify-left-crash.html:11:
document.execCommand("italic", false , null);
On 2015/03/10 05:27:07, Yosi_UTC9 wrote:
> nit: We don't need to have |false| and |null|. These are default parameter
> values.

Done.

https://codereview.chromium.org/960353002/diff/20001/LayoutTests/editing/styl...
LayoutTests/editing/style/justify-left-crash.html:12:
document.execCommand("justifyLeft", false , null);
On 2015/03/10 05:27:07, Yosi_UTC9 wrote:
> nit: We don't need to have |false| and |null|. These are default parameter
> values.

Done.

https://codereview.chromium.org/960353002/diff/20001/LayoutTests/editing/styl...
LayoutTests/editing/style/justify-left-crash.html:13: </script>
On 2015/03/10 05:27:07, Yosi_UTC9 wrote:
> Could you add output of this test to avoid empty expectation?
> We usually print "PASS if Blink doesn't crash." or so.

I tried but it is hard, the hidden attribute in html makes the whole document
hidden.

https://codereview.chromium.org/960353002/diff/20001/Source/core/editing/Edit...
File Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/960353002/diff/20001/Source/core/editing/Edit...
Source/core/editing/EditingStyle.cpp:1431:
RefPtrWillBeRawPtr<CSSComputedStyleDeclaration> computedStyle =
position.computedStyle();
On 2015/03/10 05:27:07, Yosi_UTC9 wrote:
> Where |position| does point? Does |point| point to |Document|?
> Since |Position::computedStyle()| returns computed style of element, it should
> not be null.
> Call sites, in this case |ApplyStyleCommand::applyBlockStyle|, should not try
to
> extract style for |Position| which doesn't have style.

Indeed position is a Document in the case where computedStyle returns null.

[yosin_UTC9, 2015-03-11 01:07:32]

https://codereview.chromium.org/960353002/diff/40001/LayoutTests/editing/styl...
File LayoutTests/editing/style/justify-left-crash.html (right):

https://codereview.chromium.org/960353002/diff/40001/LayoutTests/editing/styl...
LayoutTests/editing/style/justify-left-crash.html:2: <html hidden>
How about removing "hidden" attribute after executing "jusifyLeft" command? This
test script intents to check not to be crashed after "justifyLeft". Once, we
executed "justifyLeft", we can change HTML document what we want.

https://codereview.chromium.org/960353002/diff/40001/Source/core/editing/Edit...
File Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/960353002/diff/40001/Source/core/editing/Edit...
Source/core/editing/EditingStyle.cpp:1431: if (!position.element())
nit: we can put this test into L1428.

[rwlbuis, 2015-03-11 19:19:18]

https://codereview.chromium.org/960353002/diff/40001/LayoutTests/editing/styl...
File LayoutTests/editing/style/justify-left-crash.html (right):

https://codereview.chromium.org/960353002/diff/40001/LayoutTests/editing/styl...
LayoutTests/editing/style/justify-left-crash.html:2: <html hidden>
On 2015/03/11 01:07:31, Yosi_UTC9 wrote:
> How about removing "hidden" attribute after executing "jusifyLeft" command?
This
> test script intents to check not to be crashed after "justifyLeft". Once, we
> executed "justifyLeft", we can change HTML document what we want.

Clever! I did that in my latest patch set.

https://codereview.chromium.org/960353002/diff/40001/Source/core/editing/Edit...
File Source/core/editing/EditingStyle.cpp (right):

https://codereview.chromium.org/960353002/diff/40001/Source/core/editing/Edit...
Source/core/editing/EditingStyle.cpp:1431: if (!position.element())
On 2015/03/11 01:07:32, Yosi_UTC9 wrote:
> nit: we can put this test into L1428.

Done.

[yosin_UTC9, 2015-03-12 04:13:35]

yosin@chromium.org changed reviewers:
+ yutak@chromium.org

[yosin_UTC9, 2015-03-12 04:13:36]

lgtm
+yutak@ for OWNERS review

https://codereview.chromium.org/960353002/diff/60001/LayoutTests/editing/styl...
File LayoutTests/editing/style/justify-left-crash.html (right):

https://codereview.chromium.org/960353002/diff/60001/LayoutTests/editing/styl...
LayoutTests/editing/style/justify-left-crash.html:11: document.designMode =
'on';
nit: Let's use double quote, since other statements use double quote.

[Yuta Kitamura, 2015-03-12 06:01:39]

LGTM with nits

https://codereview.chromium.org/960353002/diff/60001/LayoutTests/editing/styl...
File LayoutTests/editing/style/justify-left-crash.html (right):

https://codereview.chromium.org/960353002/diff/60001/LayoutTests/editing/styl...
LayoutTests/editing/style/justify-left-crash.html:16: </script>
We generally follow the Blink C++ coding style in layout tests, too
(e.g. 4-space indent etc.).

[rwlbuis, 2015-03-12 14:11:48]

The CQ bit was checked by rob.buis@samsung.com

[rwlbuis, 2015-03-12 14:11:49]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org,
yutak@chromium.org
Link to the patchset: https://codereview.chromium.org/960353002/#ps80001 (title:
"Patch for landing")

[commit-bot: I haz the power, 2015-03-12 14:12:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/960353002/80001

[commit-bot: I haz the power, 2015-03-12 15:50:31]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191779