Remove default argument on ComputedStyle::setHasViewportUnits().

Remove default argument on ComputedStyle::setHasViewportUnits().

Currently ComputedStyle::setHasViewportUnits() takes a boolean argument,
which defaults to true. We would like to generate this field in a future
patch, but we can't generate this type of setter yet. This patch removes
the default argument on setHasViewportUnits() so that this setter can be
generated without changing the generator. All the callers are updated to
explicitly specify an argument as well.

This is prework for generating setHasViewportUnits.

BUG=628043
Review-Url: https://codereview.chromium.org/2767853002
Cr-Commit-Position: refs/heads/master@{#462270}
Committed: https://chromium.googlesource.com/chromium/src/+/369fddb8128986e11c9485342635999297d97e96

[shend, 2017-03-22 05:10:39]

shend@chromium.org changed reviewers:
+ rjwright@chromium.org

[shend, 2017-03-22 05:10:40]

Hi Renee, PTAL :)

[rune, 2017-03-22 08:59:50]

rune@opera.com changed reviewers:
+ rune@opera.com

[rune, 2017-03-22 08:59:51]

The only reason we have a parameter here is a hack used by the
ViewportStyleResolver. Ideally, this method should not have a parameter at all.

[rune, 2017-03-22 09:11:06]

Looking at the code now, we can probably afford cloning the documentStyle once
per ViewportStyleResolver::resolve and check the hasViewportUnits() of that
ComputedStyle at the end of resolve().

[shend, 2017-03-22 22:42:55]

Description was changed from

==========
Remove default argument on ComputedStyle::setHasViewportUnits().

Currently ComputedStyle::setHasViewportUnits() takes a boolean argument,
which defaults to true. We would like to generate this field in a future
patch, but we can't generate this type of setter yet. This patch removes
the default argument on setHasViewportUnits() so that this setter can be
generated without changing the generator. All the callers are updated to
explicitly specify an argument as well.

BUG=628043
==========

to

==========
Remove default argument on ComputedStyle::setHasViewportUnits().

Currently ComputedStyle::setHasViewportUnits() takes a boolean argument,
which defaults to true. We would like to generate this field in a future
patch, but we can't generate this type of setter yet. This patch removes
the default argument on setHasViewportUnits() so that this setter can be
generated without changing the generator. All the callers are updated to
explicitly specify an argument as well.

This is prework for generating setHasViewportUnits.

BUG=628043
==========

[shend, 2017-03-22 22:47:20]

On 2017/03/22 at 08:59:51, rune wrote:
> The only reason we have a parameter here is a hack used by the
ViewportStyleResolver. Ideally, this method should not have a parameter at all.

Hi Rune, thanks for the heads up. Just to check my understanding of the code:
this hack is used to check if a CSSPrimitiveValue uses viewport units? We first
save the current value of hasViewportUnits, then we reset it to false, then we
resolve lengths etc. which would set the flag to true again if there were
viewport units, and finally we reset hasViewportUnits to its original value?

If so, we could either clone the documentStyle as you suggested, or refactor
length resolution to not modify ComputedStyle (and instead returning a Result
object that tells you whether there were any viewport units, if that makes
sense).

Either way, I'd prefer landing this patch first so we can generate
hasViewportUnits, and then work on getting rid of this hack. Does that sound
good?

[rune, 2017-03-23 08:47:19]

On 2017/03/22 22:47:20, shend wrote:
> On 2017/03/22 at 08:59:51, rune wrote:
> > The only reason we have a parameter here is a hack used by the
> ViewportStyleResolver. Ideally, this method should not have a parameter at
all.
> 
> Hi Rune, thanks for the heads up. Just to check my understanding of the code:
> this hack is used to check if a CSSPrimitiveValue uses viewport units? We
first
> save the current value of hasViewportUnits, then we reset it to false, then we
> resolve lengths etc. which would set the flag to true again if there were
> viewport units, and finally we reset hasViewportUnits to its original value?

Yes, it checks whether viewport units are used by @viewport lengths. In fact, I
don't think documentStyle will ever have that flag set from other sources.

> If so, we could either clone the documentStyle as you suggested, or refactor
> length resolution to not modify ComputedStyle (and instead returning a Result
> object that tells you whether there were any viewport units, if that makes
> sense).

Yes, it's not straight-forward as CSSToLengthConversionData is const and you
need to go a couple of frames up to find StyleResolverState, for instance.

Another solution is to use the same method for @viewport as for media queries:
MediaValues::computeLengthImpl() (note it has a FIXME about using
CSSToLengthConversionData instead). Using the same for @viewport and @media
makes sense as they are resolving relative lengths against the same base apart
from viewport.

> Either way, I'd prefer landing this patch first so we can generate
> hasViewportUnits, and then work on getting rid of this hack. Does that sound
> good?

Fair enough.

[rune, 2017-03-23 08:49:30]

lgtm

[shend, 2017-04-03 04:22:33]

shend@chromium.org changed reviewers:
- rjwright@chromium.org

[shend, 2017-04-05 06:01:34]

The CQ bit was checked by shend@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 06:02:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 06:12:21]

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

[commit-bot: I haz the power, 2017-04-05 06:12:22]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[shend, 2017-04-05 06:18:45]

The CQ bit was checked by shend@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-05 06:18:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 08:01:12]

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

[commit-bot: I haz the power, 2017-04-05 08:01:13]

Dry run: This issue passed the CQ dry run.

[shend, 2017-04-05 23:13:06]

The CQ bit was checked by shend@chromium.org

[shend, 2017-04-05 23:13:06]

The patchset sent to the CQ was uploaded after l-g-t-m from rune@opera.com
Link to the patchset: https://codereview.chromium.org/2767853002/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2017-04-05 23:13:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 23:29:19]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491433986420610,
"parent_rev": "eafe1fab6a88585423399fcac64610db70ea5a04", "commit_rev":
"369fddb8128986e11c9485342635999297d97e96"}

[commit-bot: I haz the power, 2017-04-05 23:30:09]

Description was changed from

==========
Remove default argument on ComputedStyle::setHasViewportUnits().

Currently ComputedStyle::setHasViewportUnits() takes a boolean argument,
which defaults to true. We would like to generate this field in a future
patch, but we can't generate this type of setter yet. This patch removes
the default argument on setHasViewportUnits() so that this setter can be
generated without changing the generator. All the callers are updated to
explicitly specify an argument as well.

This is prework for generating setHasViewportUnits.

BUG=628043
==========

to

==========
Remove default argument on ComputedStyle::setHasViewportUnits().

Currently ComputedStyle::setHasViewportUnits() takes a boolean argument,
which defaults to true. We would like to generate this field in a future
patch, but we can't generate this type of setter yet. This patch removes
the default argument on setHasViewportUnits() so that this setter can be
generated without changing the generator. All the callers are updated to
explicitly specify an argument as well.

This is prework for generating setHasViewportUnits.

BUG=628043

Review-Url: https://codereview.chromium.org/2767853002
Cr-Commit-Position: refs/heads/master@{#462270}
Committed:
https://chromium.googlesource.com/chromium/src/+/369fddb8128986e11c9485342635...
==========

[commit-bot: I haz the power, 2017-04-05 23:30:10]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/369fddb8128986e11c9485342635...