[Webkit-unassigned] [Bug 50098] New webkit-patch rebaseline2 command.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=50098


Ojan Vafai <ojan at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dpranke at chromium.org




--- Comment #10 from Ojan Vafai <ojan at chromium.org>  2010-12-01 09:42:19 PST ---
> > > > WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/rebaseliner.py:61
> > > > +    def _make_bucket_tree(self):
> > > > +        b = bucket.BucketTree()
> > > > +        b.b(None,
> > > > +            b.b('mac',
> > > > +                b.b('mac-snowleopard',
> > > > +                    b.b('mac-leopard',
> > > > +                        b.b('mac-tiger'))),
> > > > +                b.b('win',
> > > > +                    b.b('chromium',
> > > > +                        b.b('chromium-mac'),

It's a little more complicated than this. chromium-mac falls back to chromium, then does the same fallback order as upstream mac (i.e. !win). See http://codesearch.google.com/codesearch/p#OAMlx_jo-ck/src/third_party/WebKit/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_mac.py&l=52&exact_package=chromium.

> > > > +                        b.b('chromium-win',
> > > > +                            b.b('chromium-win-vista',
> > > > +                                b.b('chromium-win-xp')),
> > > > +                            b.b('chromium-linux',
> > > > +                                b.b('chromium-gpu',
> > > > +                                    b.b('chromium-gpu-linux'),
> > > > +                                    b.b('chromium-gpu-win'),
> > > > +                                    b.b('chromium-gpu-mac'))))))),

Each platform gpu one should fallback to chromium-gpu and then the equivalent platform non-gpu one. See http://codesearch.google.com/codesearch/p#OAMlx_jo-ck/src/third_party/WebKit/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_gpu.py&l=99&exact_package=chromium

> > > > +            b.b('qt',
> > > > +                b.b('qt-linux'),
> > > > +                b.b('qt-mac'),
> > > > +                b.b('qt-win')),
> > > > +            b.b('gtk'))
> > > 
> > > Is this some kind of description of the structure of the expectations fallback tree?  If so, it should be in code that's shareable with the rest of webkitpy instead of siloed away here.
> > 
> > Yes, this is the fallback tree that is implied by the various implementations of Port.baseline_search_path() - it is needed to do correct deduping. I would like to see all the different implementations of that method go away and have the tree above be the authoritive fallback tree, but that seemed like it should happen in its own patch.
> 
> It's certainly fine to do that in a separate patch, but we're not going to want to make the situation worse by landing this patch before that one.  That's how we accumulate these technical debts.
> 
> > > > WebKitTools/Scripts/webkitpy/layout_tests/rebaseline/rebaseliner.py:63
> > > > +        AGGREGATE_BUCKETS = ['chromium', 'chromium-win', 'win', 'mac', 'qt', 'chromium-gpu']
> > > 
> > > Why is this magic list here?  We shouldn't have magic constants like this in the code.
> > 
> > These are the names of platforms that aren't targeted by any builders because they don't denote real platforms, merely convenient fallback targets. This is significant in deduping: if we have the same expectations in mac-leopard, mac-snowleopard and mac-tiger, but a different result in mac, then we can clobber the result in mac and delete the others. We couldn't do that if mac was a platform in itself, because then it having different expectations would be valid and not a duplication.
> > 
> > I've added a clarifying comment to explain the meaning of the list.
> 
> Ok, but how does that scale when we add more ports / platforms to the project?  Will someone have to hunt around all of webkitpy to find all the port-specific logic and update it?  It seems better to centralize that knowledge in the port abstraction.

I agree that this logic should be in shared code, probably in the port abstraction. I like this better than what we have in port right now though. In order to understand each platform's fallback rules, you need to look in a bunch of different files and it's very complicated. Having the data in one file would be great. That said, this could just be the underlying data used by the existing port code. For example, the base port class could have this mapping and the baseline_search_path implementations could just index into the mapping. 

What we lose here is that each port file completely encapsulates the data for that port, but the increased code clarity is worth it IMO.

In either case, that seems like it could easily be a separate patch. In the interested of avoiding too much yak shaving, how about you rewrite this patch to use baseline_search_path and leave refactoring this stuff as a FIXME?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.