Bug 73226 – make chromium layout_tests observe use_skia_on_mac.gypi from chromium/build

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

Make chromium_mac port look at chromium's src/build/use_skia_on_mac.gypi to determine whether Skia is turned on by default.

Created attachment 116775 [details]
Patch

Comment on attachment 116775 [details]
Patch

This patch won't work until http://codereview.chromium.org/8716011/ ('Move use_skia_on_mac setting into its own gypi file, so that WebKit scripts can') has landed in Chromium and been deps-rolled into WebKit.  I will update this bug once all that has happened.

[Adam Barth]

Comment on attachment 116775 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116775&action=review

> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:59
> +def is_using_skia_by_default():
> +    """Returns True if the Chromium build sets use_skia=1 by default."""
> +    gypi_path = os.path.join(
> +        os.path.dirname(__file__),
> +        os.path.pardir, os.path.pardir, os.path.pardir, os.path.pardir, os.path.pardir,
> +        'Source', 'WebKit', 'chromium', 'build', 'use_skia_on_mac.gypi')
> +    # If this open() fails with "No such file or directory", make sure that you
> +    # have updated the Chromium DEPS within your WebKit tree.
> +    # (Run update-webkit --chromium to pick up chromium/build/use_skia_on_mac.gypi )
> +    with open(gypi_path) as gypi_file:
> +        use_skia_dict = eval(gypi_file.read())
> +        if use_skia_dict['use_skia_on_mac%'] == 1:
> +            return True
> +        else:
> +            return False

Woah, crazy.  This seems like something of a hack.

(In reply to comment #3)
> Woah, crazy.  This seems like something of a hack.

I suppose so.  But maybe it is a good hack...

Given that the correct layout test behavior depends on this Chromium build flag, it seems to me that the layout test code should depend on that same build flag.  Otherwise, we have to deal with mismatching expectations until deps rolls happen (in both directions).

Is there a better way to make this change happen in sync?

[Adam Barth]

Maybe the best course of action is to add a FIXME comment about this code just being useful for the transition and saying it should be removed when such-and-such happens.

(In reply to comment #5)
> Maybe the best course of action is to add a FIXME comment about this code just being useful for the transition and saying it should be removed when such-and-such happens.

That certainly sounds reasonable to me.

If/when the chromium CL lands, I will make that change (along with any other changes) and ping you.

[Eric Seidel (no email)]

Comment on attachment 116775 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116775&action=review

> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:50
> +    gypi_path = os.path.join(
> +        os.path.dirname(__file__),
> +        os.path.pardir, os.path.pardir, os.path.pardir, os.path.pardir, os.path.pardir,
> +        'Source', 'WebKit', 'chromium', 'build', 'use_skia_on_mac.gypi')

Filesystem, please.  Also Checkout can point you to the chromium directory I believe.  Certainly Port can.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:54
> +    with open(gypi_path) as gypi_file:

Please use FileSystem.  Please move this to a class so it can have a pointer to a Host/FileSystem so it can be mocked and tested.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:55
> +        use_skia_dict = eval(gypi_file.read())

Is this the expected way to parse gypi's?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:58
> +        if use_skia_dict['use_skia_on_mac%'] == 1:
> +            return True
> +        else:

This could just be return use_skia_dict['use_skia_on_mac%'] == 1 instead of this 4-line if.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:59
>> +            return False
> 
> Woah, crazy.  This seems like something of a hack.

This is a huge hack. :(  Doubly so that it's a free function. :(

> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac_unittest.py:54
> +        if chromium_mac.is_using_skia_by_default():
> +            self.assertTrue(self.make_port().name() in (
> +                'chromium-mac-leopard', 'chromium-mac-snowleopard', 'chromium-mac-lion', 'chromium-mac-future'))
> +        else:
> +            self.assertTrue(self.make_port().name() in (
> +                'chromium-cg-mac-leopard', 'chromium-cg-mac-snowleopard', 'chromium-cg-mac-lion', 'chromium-cg-mac-future'))

No, no.  Please do not make the unittests depend on the environment anymore than they already do.

(In reply to comment #7)

Thanks for the feedback, Eric; I will try to incorporate the specific changes you requested and update the bug when I have done so.

> >> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:59
> >> +            return False
> > 
> > Woah, crazy.  This seems like something of a hack.
> 
> This is a huge hack. :(  Doubly so that it's a free function. :(
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium_mac_unittest.py:54
> > +        if chromium_mac.is_using_skia_by_default():
> > +            self.assertTrue(self.make_port().name() in (
> > +                'chromium-mac-leopard', 'chromium-mac-snowleopard', 'chromium-mac-lion', 'chromium-mac-future'))
> > +        else:
> > +            self.assertTrue(self.make_port().name() in (
> > +                'chromium-cg-mac-leopard', 'chromium-cg-mac-snowleopard', 'chromium-cg-mac-lion', 'chromium-cg-mac-future'))
> 
> No, no.  Please do not make the unittests depend on the environment anymore than they already do.

Stepping back a bit, is there a preferable way to accomplish the overarching goal here?  We need some way of synchronizing the chromium and webkit trees, because the chromium tree decides whether to use CG or Skia and that affects the results of NRWT (in the webkit tree).

It's not really an "environment" parameter... it's a characteristic of the DumpRenderTree binary that is being tested by NRWT.

I suppose I could add a flag to DumpRenderTree that would make it report whether it was built using Skia or CG, and NRWT could use that information rather than looking in the chromium build configuration.  Would that be preferable?

Or maybe there is some other clever way of synchronizing this?

[Tony Chang]

A couple less hacky suggestions:

1) Run nm on DumpRenderTree and grep for a symbol that's only in Skia builds.
2) Update the gyp files so that if use_skia is true, it writes a file to disk with the word "skia" or "cg" in it.  NRWT can read this file.

[Tony Chang]

(In reply to comment #8)
> I suppose I could add a flag to DumpRenderTree that would make it report whether it was built using Skia or CG, and NRWT could use that information rather than looking in the chromium build configuration.  Would that be preferable?

Oh, I didn't see this.  This seems fine too.

(In reply to comment #10)
> (In reply to comment #8)
> > I suppose I could add a flag to DumpRenderTree that would make it report whether it was built using Skia or CG, and NRWT could use that information rather than looking in the chromium build configuration.  Would that be preferable?
> 
> Oh, I didn't see this.  This seems fine too.

Do others agree that it is better to depend on the DumpRenderTree binary rather than the gyp that was used to build it?  If so, I can go down that road instead.

Created attachment 117432 [details]
Patch

Comment on attachment 116775 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116775&action=review

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:50
>> +        'Source', 'WebKit', 'chromium', 'build', 'use_skia_on_mac.gypi')
> 
> Filesystem, please.  Also Checkout can point you to the chromium directory I believe.  Certainly Port can.

Now using filesystem.  See new comment indicating why I am unable to call ChromiumPort.path_from_chromium_base() to get the path.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:54
>> +    with open(gypi_path) as gypi_file:
> 
> Please use FileSystem.  Please move this to a class so it can have a pointer to a Host/FileSystem so it can be mocked and tested.

Using filesystem. I'd prefer to not go to the trouble of adding a new class (or adding unittests), since it's just a temporary thing while we transition to use_skia=1.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:55
>> +        use_skia_dict = eval(gypi_file.read())
> 
> Is this the expected way to parse gypi's?

Yes. Added documentation about this.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:58
>> +        else:
> 
> This could just be return use_skia_dict['use_skia_on_mac%'] == 1 instead of this 4-line if.

Done.

>>> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:59
>>> +            return False
>> 
>> Woah, crazy.  This seems like something of a hack.
> 
> This is a huge hack. :(  Doubly so that it's a free function. :(

Less hacky now, but admittedly still on the hacky side... but I think it's a fair tradeoff in order to keep things green during a transition period.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac_unittest.py:54
>> +                'chromium-cg-mac-leopard', 'chromium-cg-mac-snowleopard', 'chromium-cg-mac-lion', 'chromium-cg-mac-future'))
> 
> No, no.  Please do not make the unittests depend on the environment anymore than they already do.

Done.

abarth/eric/tony: please see new patch.  I'm still parsing the gypi to see if use_skia=1 , but given that this is just a transitional thing I think it's a pretty good balance.  If you still have objections, maybe we can talk live to figure out a reasonably simple way to deal with the transition phase?

abarth and I have been discussing this on #webkit.  synopsis:

abarth says: why bother with this when we can just "flip the switch" to turn skia on in Chrome, and then live with red buildbots until chromium->webkit and webkit->chromium deps rolls are completed a few hours later?

epoger says: I think it's more likely for those deps rolls to take a day or two to complete, and given that this temporary "hack" will prevent that synchronization problem, why not take advantage of that and keep the buildbots green?

Adam and Eric: I must say I am mystified by this preference for breaking developers' layout_test results rather than allowing "hacky" code into webkit for a few days.  But you guys are WebKit reviewers and I'm not, so I will let you decide.  Please let me know what approach you want me to take, and I'll take it:

a) commit this patch so layout_test results stay valid during the transition phase, but in a "hacky" way
b) commit the use_skia=1 change to chromium, knowing that it will break layout_test results, and then try to get webkit->chromium and chromium->webkit deps rolls done ASAP
c) take some other approach to keeping the buildbots green, even though that's more effort (and please tell me what approach you will find acceptable)
d) something else I haven't thought of

[Adam Barth]

I guess I don't understand why this needs to break things for more than a few hours.  Can't you just use the following plan:

1) Roll Chromium DEPS in WebKit to tip-of-tree.  This should be doable all the time.
2) Enable skia downstream.
3) Roll Chromium DEPS in WebKit to tip-of-tree again.  There shouldn't be any latency between step (2) and (3).
4) Edit webkitpy now that Skia is the default.  We can review that patch ahead of time so you can land it within minutes of step (3).

Nothing on that list should take any substantial amount of time.

(In reply to comment #16)
> I guess I don't understand why this needs to break things for more than a few hours.  Can't you just use the following plan:
> 
> 1) Roll Chromium DEPS in WebKit to tip-of-tree.  This should be doable all the time.
> 2) Enable skia downstream.
> 3) Roll Chromium DEPS in WebKit to tip-of-tree again.  There shouldn't be any latency between step (2) and (3).
> 4) Edit webkitpy now that Skia is the default.  We can review that patch ahead of time so you can land it within minutes of step (3).
> 
> Nothing on that list should take any substantial amount of time.

OK, I'll do it as a series of changes as close together as possible, rather than committing a synchronization "hack".  It's actually slightly different from how you describe (there is also a needed chromium->webkit deps roll).  Here are the steps I will take, and the state we can expect to be in after each step.

I will either start this process this morning or in about a week (after Chromium M17 branch), depending on some other conversations.  I will update this bug once I know more about the timing.


Current state: 
- chromium tree builds Chromium-Mac using CG
- webkit tree runs layout tests assuming that Chromium-Mac uses CG

1) Roll Chromium DEPS in WebKit (elsewhere I have referred to this as "webkit->chromium deps") to tip-of-tree.  I actually did this yesterday, in https://bugs.webkit.org/show_bug.cgi?id=73231 ('webkit->chromium DEPS roll 111575->112463').

State is unchanged.

2) Change chromium tree to build Chromium-Mac using Skia.

State:
- chromium.webkit bots will fail because layout tests (brought in from webkit) assume CG
- build.webkit.org bots will stay green because they haven't picked up the change in chromium yet

3) Update webkitpy scripts to assume Chromium-Mac uses Skia AND roll Chromium DEPS in WebKit, in the same commit.

State:
- chromium.webkit "canary" bots will go green because webkit tip-of-tree now agrees with Chromium again
- chromium.webkit "deps" bots will be red until we roll WebKit DEPS in Chromium to pick up new layout_test assumptions
- build.webkit.org bots will stay green because Chromium build and layout_test assumptions changed simultaneously

4) Roll WebKit DEPS in Chromium to tip-of-tree to pick up layout_test changes.

State: all buildbots should be green.

(In reply to comment #17)
> I will either start this process this morning or in about a week (after Chromium M17 branch), depending on some other conversations.  I will update this bug once I know more about the timing.

M17 branch is slated for the first part of next week; we will wait until after that.

Unless I hear otherwise, I will abandon the synchronization "hack" patch in this bug, and just follow the steps Adam and I described above.

(In reply to comment #18)
> (In reply to comment #17)
> > I will either start this process this morning or in about a week (after Chromium M17 branch), depending on some other conversations.  I will update this bug once I know more about the timing.
> 
> M17 branch is slated for the first part of next week; we will wait until after that.
> 
> Unless I hear otherwise, I will abandon the synchronization "hack" patch in this bug, and just follow the steps Adam and I described above.

P.S. Progress in "flipping the switch" will be tracked in http://crbug.com/101731 ('re-enable use_skia by default').

[Eric Seidel (no email)]

Comment on attachment 117432 [details]
Patch

Isn't the CG port dead?  Does this patch still make sense?

[Adam Barth]

Comment on attachment 117432 [details]
Patch

Chromium-CG is gone.