Move rem handling out of CSSParserValues.cpp

Move rem handling out of CSSParserValues

Move rem handling out of CSSParserValues and instead keep track of
whether rem units were applied per ComputedStyle. Then once styleForElement
is finished if rem units were used we inform StyleEngine.

Add rem-calc-dynamic-scaling.html for using rem with calc().

BUG=499780, 515611
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=199770

[rwlbuis, 2015-06-17 00:51:22]

rob.buis@samsung.com changed reviewers:
+ timloh@chromium.org

[rwlbuis, 2015-06-17 00:51:22]

PTAL

[Timothy Loh, 2015-06-17 01:35:18]

https://codereview.chromium.org/1181023004/diff/1/Source/core/css/parser/CSSP...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1181023004/diff/1/Source/core/css/parser/CSSP...
Source/core/css/parser/CSSPropertyParser.cpp:388: if (value->unit ==
CSSPrimitiveValue::CSS_REMS) {
Probably better in createPrimitiveNumericValue, I think there's code which calls
that directly

[Timothy Loh, 2015-06-17 03:27:53]

Err I don't think this works for calc :/

[rwlbuis, 2015-07-29 20:09:52]

Patchset #4 (id:60001) has been deleted

[rwlbuis, 2015-07-29 21:46:27]

On 2015/06/17 03:27:53, Timothy Loh wrote:
> Err I don't think this works for calc :/

PTAL. I somehow overlooked the viewportUnits code I think, I use the same
approach now.

Note that this is rather simple and does not handle dynamic changes (i.e. once a
rem is applied the flag will stay on in StyleEngine, even if later that rule is
removed) but I don't think viewportUnits handle it either, and having the root
element change its font-size should be rare, as the comment in
Document::inheritHtmlAndBodyElementStyles mentions.

[Timothy Loh, 2015-07-30 01:06:55]

lgtm!

https://codereview.chromium.org/1181023004/diff/80001/LayoutTests/fast/css/re...
File LayoutTests/fast/css/rem-dynamic-scaling.html (right):

https://codereview.chromium.org/1181023004/diff/80001/LayoutTests/fast/css/re...
LayoutTests/fast/css/rem-dynamic-scaling.html:9: <div style="font-size:calc(0% +
1rem)">
Uhhh.. put this in a separate test, since if it didn't mark the style as having
rem units, the other div probably would.

https://codereview.chromium.org/1181023004/diff/80001/Source/core/style/Compu...
File Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1181023004/diff/80001/Source/core/style/Compu...
Source/core/style/ComputedStyle.h:387: void setHasRemUnits(bool hasRemUnits =
true) const { noninherited_flags.hasRemUnits = hasRemUnits; }
Probably no point having an argument (even though the viewport units one has
one)

[rwlbuis, 2015-07-30 13:13:21]

Patchset #5 (id:100001) has been deleted

[rwlbuis, 2015-07-30 13:15:22]

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

[rwlbuis, 2015-07-30 13:15:22]

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

[commit-bot: I haz the power, 2015-07-30 13:15:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1181023004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1181023004/120001

[commit-bot: I haz the power, 2015-07-30 14:38:24]

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

[dmurph, 2015-07-30 16:27:21]

dmurph@chromium.org changed reviewers:
+ dmurph@chromium.org

[dmurph, 2015-07-30 16:27:22]

I think this killed MSAN bot.  Unfortunately I need to revert manually.
https://code.google.com/p/chromium/issues/detail?id=515535

[rwlbuis, 2015-07-30 17:35:03]

On 2015/07/30 16:27:22, dmurph wrote:
> I think this killed MSAN bot.  Unfortunately I need to revert manually.
> https://code.google.com/p/chromium/issues/detail?id=515535

Ok, is there some easy way to test for this kind of problem? Any bot?

[blink-reviews, 2015-07-30 17:37:16]

Is there a memory sanatizer trybot?  You might need to build with memsan
enabled, and then try the tests that failed.

On Thu, Jul 30, 2015 at 6:35 PM <rob.buis@samsung.com> wrote:

> On 2015/07/30 16:27:22, dmurph wrote:
> > I think this killed MSAN bot.  Unfortunately I need to revert manually.
> > https://code.google.com/p/chromium/issues/detail?id=515535
>
> Ok, is there some easy way to test for this kind of problem? Any bot?
>
> https://codereview.chromium.org/1181023004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[dmurph, 2015-07-30 17:37:44]

Is there a memory sanatizer trybot?  You might need to build with memsan
enabled, and then try the tests that failed.

https://www.chromium.org/developers/testing/memorysanitizer

On Thu, Jul 30, 2015 at 6:35 PM <rob.buis@samsung.com> wrote:

> On 2015/07/30 16:27:22, dmurph wrote:
> > I think this killed MSAN bot.  Unfortunately I need to revert manually.
> > https://code.google.com/p/chromium/issues/detail?id=515535
>
> Ok, is there some easy way to test for this kind of problem? Any bot?
>
> https://codereview.chromium.org/1181023004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[rwlbuis, 2015-07-30 17:47:42]

On 2015/07/30 17:37:44, dmurph wrote:
> Is there a memory sanatizer trybot?  You might need to build with memsan
> enabled, and then try the tests that failed.
> 
> https://www.chromium.org/developers/testing/memorysanitizer

Thanks! I am trying linux_chromium_msan_rel_ng first. I think I have a fix
too...

[Lei Zhang, 2015-07-30 18:37:48]

thestig@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2015-07-30 18:37:49]

While verifying http://crbug.com/514925, I got new uninit memory errors here:

UninitCondition
Conditional jump or move depends on uninitialised value(s)
  blink::StyleResolver::styleForElement(blink::Element*, blink::ComputedStyle
const*, blink::StyleSharingBehavior, blink::RuleMatchingBehavior)
(third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:637)
  blink::Element::originalStyleForLayoutObject()
(/usr/local/google/home/thestig/chrome.git/src/out/Release/content_shell)
  blink::Element::styleForLayoutObject()
(third_party/WebKit/Source/core/dom/Element.cpp:1595)
  blink::Element::recalcOwnStyle(blink::StyleRecalcChange)
(third_party/WebKit/Source/core/dom/Element.cpp:1688)

...

Did you forget to initialize |hasRemUnits| ?

[rwlbuis, 2015-07-30 19:12:09]

On 2015/07/30 18:37:49, Lei Zhang wrote:
> While verifying http://crbug.com/514925, I got new uninit memory errors here:
> 
> UninitCondition
> Conditional jump or move depends on uninitialised value(s)
>   blink::StyleResolver::styleForElement(blink::Element*, blink::ComputedStyle
> const*, blink::StyleSharingBehavior, blink::RuleMatchingBehavior)
> (third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:637)
>   blink::Element::originalStyleForLayoutObject()
> (/usr/local/google/home/thestig/chrome.git/src/out/Release/content_shell)
>   blink::Element::styleForLayoutObject()
> (third_party/WebKit/Source/core/dom/Element.cpp:1595)
>   blink::Element::recalcOwnStyle(blink::StyleRecalcChange)
> (third_party/WebKit/Source/core/dom/Element.cpp:1688)
> 
> ...
> 
> Did you forget to initialize |hasRemUnits| ?

Yep. I thought I had copied all places where viewportUnits was used in
ComputedStyle.h but I forgot one place. Will fix soon.

[rwlbuis, 2015-07-30 21:35:55]

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

[rwlbuis, 2015-07-30 21:35:55]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1181023004/#ps140001
(title: "Patch for relanding")

[commit-bot: I haz the power, 2015-07-30 21:36:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1181023004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1181023004/140001

[commit-bot: I haz the power, 2015-07-30 22:22:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-30 22:22:29]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...)

[rwlbuis, 2015-07-30 23:54:41]

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

[rwlbuis, 2015-07-30 23:54:41]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1181023004/#ps160001
(title: "Another attempt")

[commit-bot: I haz the power, 2015-07-30 23:54:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1181023004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1181023004/160001

[commit-bot: I haz the power, 2015-07-31 00:59:12]

Committed patchset #7 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199770