Bug 39721 – [chromium] Upstream layout tests expectations for Geolocation.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Marcus Bulach]

Geolocation is now supported on both DumpRenderTree and test_shell, need to upstream the test expectations.

[Marcus Bulach]

Created attachment 57093 [details]
Patch

[Jeremy Orlow]

Comment on attachment 57093 [details]
Patch

LayoutTests/platform/chromium/test_expectations.txt:709
 +  BUG11246 DEFER SKIP : fast/dom/Geolocation/callback-exception.html = TEXT
Just rebaseline it in this patch.  Use the rebaseline tool.

[Marcus Bulach]

Created attachment 57324 [details]
Patch

[Marcus Bulach]

(In reply to comment #2)
> (From update of attachment 57093 [details])
> LayoutTests/platform/chromium/test_expectations.txt:709
>  +  BUG11246 DEFER SKIP : fast/dom/Geolocation/callback-exception.html = TEXT
> Just rebaseline it in this patch.  Use the rebaseline tool.

duh, of course! rebaselined. another look please?

[Jeremy Orlow]

Comment on attachment 57324 [details]
Patch

LayoutTests/platform/chromium/fast/dom/Geolocation/callback-exception-expected.txt:1
 +  CONSOLE MESSAGE: line 22: Uncaught Error: Exception in success callback
This worries me some.  Does it show up with JSC?  I feel like maybe you could catch the exception in the test.

LayoutTests/ChangeLog:9
 +          * platform/chromium/fast/dom/Geolocation/callback-exception-expected.txt: Added.
Does this work?  I believe you have to add it to -win and -mac.  Dimitri has confirmed this in the past.  But, now that I think about it, maybe that's silly?  Especially if this works.  (Are you sure it does?)

[Jeremy Orlow]

Dimitri, can you take a quick look?

[Jeremy Orlow]

(In reply to comment #5)
> (From update of attachment 57324 [details])
> LayoutTests/platform/chromium/fast/dom/Geolocation/callback-exception-expected.txt:1
>  +  CONSOLE MESSAGE: line 22: Uncaught Error: Exception in success callback
> This worries me some.  Does it show up with JSC?  I feel like maybe you could catch the exception in the test.

Nm.  I'm an idiot.  :-)

[Marcus Bulach]

(In reply to comment #7)
> (In reply to comment #5)
> > (From update of attachment 57324 [details] [details])
> > LayoutTests/platform/chromium/fast/dom/Geolocation/callback-exception-expected.txt:1
> >  +  CONSOLE MESSAGE: line 22: Uncaught Error: Exception in success callback
> > This worries me some.  Does it show up with JSC?  I feel like maybe you could catch the exception in the test.
> 
> Nm.  I'm an idiot.  :-)

so the difference between V8 and JSC is the "Uncaught " word.. I could certainly change the test, but that would be a separate change..

> 
> LayoutTests/ChangeLog:9
>  +          * platform/chromium/fast/dom/Geolocation/callback-exception-expected.txt: Added.
> Does this work?  I believe you have to add it to -win and -mac.  Dimitri has confirmed this in the past.  But, now that I think about it, maybe that's silly?  Especially if this works.  (Are you sure it does?)

"works for me!" :)
dimitri, could you please confirm it'll work everywhere?

[Dimitri Glazkov (Google)]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > (From update of attachment 57324 [details] [details] [details])
> > > LayoutTests/platform/chromium/fast/dom/Geolocation/callback-exception-expected.txt:1
> > >  +  CONSOLE MESSAGE: line 22: Uncaught Error: Exception in success callback
> > > This worries me some.  Does it show up with JSC?  I feel like maybe you could catch the exception in the test.
> > 
> > Nm.  I'm an idiot.  :-)
> 
> so the difference between V8 and JSC is the "Uncaught " word.. I could certainly change the test, but that would be a separate change..
> 
> > 
> > LayoutTests/ChangeLog:9
> >  +          * platform/chromium/fast/dom/Geolocation/callback-exception-expected.txt: Added.
> > Does this work?  I believe you have to add it to -win and -mac.  Dimitri has confirmed this in the past.  But, now that I think about it, maybe that's silly?  Especially if this works.  (Are you sure it does?)
> 
> "works for me!" :)
> dimitri, could you please confirm it'll work everywhere?

Not that I am a aware. I think you still need to put 'em in -win and -mac. Ask dpranke. He knows.

[Jeremy Orlow]

If there's a good reason platform/chromium can't be in the search path for Linux, Win, and Mac, then we need to delete everything from that directory.  (IIRC, there are a bunch of other expected results in there!)

If there's not, we should probably change the re-baselining tool, send a FYI email to chromium-dev, and optionally create a script to move all the duplicate files in -mac and -win to the shared folder.

I hope the answer is the latter.  :-)

[Dirk Pranke]

(In reply to comment #10)
> If there's a good reason platform/chromium can't be in the search path for Linux, Win, and Mac, then we need to delete everything from that directory.  (IIRC, there are a bunch of other expected results in there!)
> 
> If there's not, we should probably change the re-baselining tool, send a FYI email to chromium-dev, and optionally create a script to move all the duplicate files in -mac and -win to the shared folder.
> 
> I hope the answer is the latter.  :-)

platform/chromium is in the search path on all three platforms. It's fine to put files that are the same on all three Chromium ports but different from upstream there.

I thought the rebaselining tool automatically dealt with this, but I could be wrong.

Copying victorw for thoughts on this ... (watch me pass the buck!)

[Victor Wang]

Personally, I prefer not having baselines in platform/chromium. For now, since the fallback paths are chromium-linux->chromium-win->chromium-mac, if baselines for all three ports are same, then the baseline would be in chromium-mac and chromium-mac is serving the same purpose as platform/chromium.

I think having baselines in platfomr/chromium may add extra maintenance work.
For example, if a baseline for all ports are same but latter linux has a different baseline, then in addition to add new linux baseline, we also need to remove baseline from platform/chromium and add it to platform/chromium-mac.
It makes rebaseline tool little harder when it tries to rebaseline for one platform and the tool needs to figure out where to add new baseline. I agree these issues could be resolved by making rebaseline smarter, but don't feel it worth the effort to do that, at least for now. I prefer keeping it simple and just use the existing path, thoughts?

(In reply to comment #11)
> (In reply to comment #10)
> > If there's a good reason platform/chromium can't be in the search path for Linux, Win, and Mac, then we need to delete everything from that directory.  (IIRC, there are a bunch of other expected results in there!)
> > 
> > If there's not, we should probably change the re-baselining tool, send a FYI email to chromium-dev, and optionally create a script to move all the duplicate files in -mac and -win to the shared folder.
> > 
> > I hope the answer is the latter.  :-)
> 
> platform/chromium is in the search path on all three platforms. It's fine to put files that are the same on all three Chromium ports but different from upstream there.
> 
> I thought the rebaselining tool automatically dealt with this, but I could be wrong.
> 
> Copying victorw for thoughts on this ... (watch me pass the buck!)

[Dirk Pranke]

(In reply to comment #12)
> Personally, I prefer not having baselines in platform/chromium. For now, since the fallback paths are chromium-linux->chromium-win->chromium-mac, if baselines for all three ports are same, then the baseline would be in chromium-mac and chromium-mac is serving the same purpose as platform/chromium.

Except that neither chromium-linux nor chromium-win fall back to chromium-mac.

[Marcus Bulach]

(In reply to comment #13)
> (In reply to comment #12)
> > Personally, I prefer not having baselines in platform/chromium. For now, since the fallback paths are chromium-linux->chromium-win->chromium-mac, if baselines for all three ports are same, then the baseline would be in chromium-mac and chromium-mac is serving the same purpose as platform/chromium.
> 
> Except that neither chromium-linux nor chromium-win fall back to chromium-mac.

thanks everyone for the feedback!
it seems that platform/chromium is indeed the fallback for all platforms?
should we go ahead with this patch or is it better to duplicate it and add to chromium-win and chromium-mac?

[Jeremy Orlow]

Unless there's a good reason to separate win/mac, I think we should go forward with this patch, change the default behavior of the rebaselining tool, and file a bug to unify all the tests which have identical mac/win baselines.

[Victor Wang]

(In reply to comment #15)
> Unless there's a good reason to separate win/mac, I think we should go forward with this patch, change the default behavior of the rebaselining tool, and file a bug to unify all the tests which have identical mac/win baselines.

I don't think we should put baselines in platform/chromium. We already have a baseline location for each platform and fallback logic works well through platform-linux->platform->win->platform-mac. I don't see much benefit if we add additional layer to this fallback path, but does see there is maintenance cost (either through rebaseline tool or manually update baselines) introduced by this extra location. Like I said, if we have this extra layer and baselines for all three platforms are same, the baseline is added to platform/chromium, then if the baseline for any platform changes, not only you need to add/update the baseline for that location, you may also need to update the other platforms and remove the file from platform/chromium. I don't mean we could not do this in rebaseline tool or manually rebaselining, but think this makes our rebaseline process more complicated and the benefit by adding this extra layer is not much.

[Jeremy Orlow]

(In reply to comment #16)
> (In reply to comment #15)
> > Unless there's a good reason to separate win/mac, I think we should go forward with this patch, change the default behavior of the rebaselining tool, and file a bug to unify all the tests which have identical mac/win baselines.
> 
> I don't think we should put baselines in platform/chromium. We already have a baseline location for each platform and fallback logic works well through platform-linux->platform->win->platform-mac. I don't see much benefit if we add additional layer to this fallback path, but does see there is maintenance cost (either through rebaseline tool or manually update baselines) introduced by this extra location. Like I said, if we have this extra layer and baselines for all three platforms are same, the baseline is added to platform/chromium, then if the baseline for any platform changes, not only you need to add/update the baseline for that location, you may also need to update the other platforms and remove the file from platform/chromium. I don't mean we could not do this in rebaseline tool or manually rebaselining, but think this makes our rebaseline process more complicated and the benefit by adding this extra layer is not much.

By that logic, we should not have linux fall back on the win baselines for the same reason, right?

Also, if that's the case, we need to move all the baselines out of platform/chromium to avoid further confusion.

[Victor Wang]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > Unless there's a good reason to separate win/mac, I think we should go forward with this patch, change the default behavior of the rebaselining tool, and file a bug to unify all the tests which have identical mac/win baselines.
> > 
> > I don't think we should put baselines in platform/chromium. We already have a baseline location for each platform and fallback logic works well through platform-linux->platform->win->platform-mac. I don't see much benefit if we add additional layer to this fallback path, but does see there is maintenance cost (either through rebaseline tool or manually update baselines) introduced by this extra location. Like I said, if we have this extra layer and baselines for all three platforms are same, the baseline is added to platform/chromium, then if the baseline for any platform changes, not only you need to add/update the baseline for that location, you may also need to update the other platforms and remove the file from platform/chromium. I don't mean we could not do this in rebaseline tool or manually rebaselining, but think this makes our rebaseline process more complicated and the benefit by adding this extra layer is not much.
> 
> By that logic, we should not have linux fall back on the win baselines for the same reason, right?
Jeremy, maybe I didn't make my comments clearer. I do think we should have linux fallback to win (lots of baselines for linux and win are same). The point I am trying to make here is each baseline location is associated with one platform (mac, win7, vista, winxp, linux etc). The new location platform/chromium is not associate with one platform, it is for hosting files if baselines for all platforms are same. This case has already been handled now by simply adding the baseline to platform-mac. Adding this new location will make the process MORE complicated and seems to me it is not necessary.

> 
> Also, if that's the case, we need to move all the baselines out of platform/chromium to avoid further confusion.
Yes, I agree.

[Victor Wang]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > (In reply to comment #15)
> > > > Unless there's a good reason to separate win/mac, I think we should go forward with this patch, change the default behavior of the rebaselining tool, and file a bug to unify all the tests which have identical mac/win baselines.
> > > 
> > > I don't think we should put baselines in platform/chromium. We already have a baseline location for each platform and fallback logic works well through platform-linux->platform->win->platform-mac. I don't see much benefit if we add additional layer to this fallback path, but does see there is maintenance cost (either through rebaseline tool or manually update baselines) introduced by this extra location. Like I said, if we have this extra layer and baselines for all three platforms are same, the baseline is added to platform/chromium, then if the baseline for any platform changes, not only you need to add/update the baseline for that location, you may also need to update the other platforms and remove the file from platform/chromium. I don't mean we could not do this in rebaseline tool or manually rebaselining, but think this makes our rebaseline process more complicated and the benefit by adding this extra layer is not much.
> > 
> > By that logic, we should not have linux fall back on the win baselines for the same reason, right?
> Jeremy, maybe I didn't make my comments clearer. I do think we should have linux fallback to win (lots of baselines for linux and win are same). The point I am trying to make here is each baseline location is associated with one platform (mac, win7, vista, winxp, linux etc). The new location platform/chromium is not associate with one platform, it is for hosting files if baselines for all platforms are same. This case has already been handled now by simply adding the baseline to platform-mac. Adding this new location will make the process MORE complicated and seems to me it is not necessary.
> 
> > 
> > Also, if that's the case, we need to move all the baselines out of platform/chromium to avoid further confusion.
> Yes, I agree.

I think it is more logically clear and simple if baseline for each platform are added to it own location and no dupe check if there is no storage concern, but lots of baselines are same so we need the fallback logic to save storage. Adding platform/chromium does not save us any space but does make the process more complicated.

[Dirk Pranke]

(In reply to comment #16)
> (In reply to comment #15)
> > Unless there's a good reason to separate win/mac, I think we should go forward with this patch, change the default behavior of the rebaselining tool, and file a bug to unify all the tests which have identical mac/win baselines.
> 
> I don't think we should put baselines in platform/chromium. We already have a baseline location for each platform and fallback logic works well through platform-linux->platform->win->platform-mac. 

To repeat what I said in comment #13, neither platform/chromium-win nor platform/chromium-linux  falls back to chromium-mac, which means there is no place we can put a single file that has the same expectation on all chromium ports but is different from upstream webkit.

I think it makes sense to have such a place, because that is easy to understand for people writing new tests, and that way people don't get confused if we tell them to put a result in chromium-mac even if it is the same on all three platforms.

In addition, we know that it is something of a hassle to rebaseline all three platforms (although the rebaselining tool does make it easier than it would be by hand), and it does introduce a source of error for both committers and reviewers.

So, if I put a file in platform/chromium, and the mac baseline changes, then I don't have to remove the file from platform/chromium (linux and win will still pick it up). This is no different from how the rest of the fallback code works.

Things would be a little clearer if we removed the platform/mac (not chromium-mac) baseline, so that it was clear that the win (and linux) platforms never depended on mac-specific *anything*. I think most of the mac-specific baselines picked up by the win bots are for mac-specific tests, so arguably we could stop running those as well.

However, you're right that storage isn't a concern.

[Victor Wang]

(In reply to comment #20)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > Unless there's a good reason to separate win/mac, I think we should go forward with this patch, change the default behavior of the rebaselining tool, and file a bug to unify all the tests which have identical mac/win baselines.
> > 
> > I don't think we should put baselines in platform/chromium. We already have a baseline location for each platform and fallback logic works well through platform-linux->platform->win->platform-mac. 
> 
> To repeat what I said in comment #13, neither platform/chromium-win nor platform/chromium-linux  falls back to chromium-mac, which means there is no place we can put a single file that has the same expectation on all chromium ports but is different from upstream webkit.
> 
> I think it makes sense to have such a place, because that is easy to understand for people writing new tests, and that way people don't get confused if we tell them to put a result in chromium-mac even if it is the same on all three platforms.
> 
> In addition, we know that it is something of a hassle to rebaseline all three platforms (although the rebaselining tool does make it easier than it would be by hand), and it does introduce a source of error for both committers and reviewers.
> 
> So, if I put a file in platform/chromium, and the mac baseline changes, then I don't have to remove the file from platform/chromium (linux and win will still pick it up). This is no different from how the rest of the fallback code works.
> 
> Things would be a little clearer if we removed the platform/mac (not chromium-mac) baseline, so that it was clear that the win (and linux) platforms never depended on mac-specific *anything*. I think most of the mac-specific baselines picked up by the win bots are for mac-specific tests, so arguably we could stop running those as well.
> 
> However, you're right that storage isn't a concern.

I see. Somehow, I thought the chromium-win falls back to chromium-mac as we have in downstream code. This is why I thought the new platform/chromium is not that useful. If this is not the case, I agree adding platform/chromium to fallback path is fine.

Since platform/chromium is already in search path and chromium-win or chromium-linux does not fall back to chromium-mac, the existing rebaseline tool should work as you said in your comments.

Sorry for the misunderstanding.

[Marcus Bulach]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #16)
> > > (In reply to comment #15)
> > > > Unless there's a good reason to separate win/mac, I think we should go forward with this patch, change the default behavior of the rebaselining tool, and file a bug to unify all the tests which have identical mac/win baselines.
> > > 
> > > I don't think we should put baselines in platform/chromium. We already have a baseline location for each platform and fallback logic works well through platform-linux->platform->win->platform-mac. 
> > 
> > To repeat what I said in comment #13, neither platform/chromium-win nor platform/chromium-linux  falls back to chromium-mac, which means there is no place we can put a single file that has the same expectation on all chromium ports but is different from upstream webkit.
> > 
> > I think it makes sense to have such a place, because that is easy to understand for people writing new tests, and that way people don't get confused if we tell them to put a result in chromium-mac even if it is the same on all three platforms.
> > 
> > In addition, we know that it is something of a hassle to rebaseline all three platforms (although the rebaselining tool does make it easier than it would be by hand), and it does introduce a source of error for both committers and reviewers.
> > 
> > So, if I put a file in platform/chromium, and the mac baseline changes, then I don't have to remove the file from platform/chromium (linux and win will still pick it up). This is no different from how the rest of the fallback code works.
> > 
> > Things would be a little clearer if we removed the platform/mac (not chromium-mac) baseline, so that it was clear that the win (and linux) platforms never depended on mac-specific *anything*. I think most of the mac-specific baselines picked up by the win bots are for mac-specific tests, so arguably we could stop running those as well.
> > 
> > However, you're right that storage isn't a concern.
> 
> I see. Somehow, I thought the chromium-win falls back to chromium-mac as we have in downstream code. This is why I thought the new platform/chromium is not that useful. If this is not the case, I agree adding platform/chromium to fallback path is fine.
> 
> Since platform/chromium is already in search path and chromium-win or chromium-linux does not fall back to chromium-mac, the existing rebaseline tool should work as you said in your comments.
> 
> Sorry for the misunderstanding.

thanks all for the clarifications!
so should we go with this patch as is then?! :)

[Jeremy Orlow]

Comment on attachment 57324 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 57324 [details]
Patch

Clearing flags on attachment: 57324

Committed r60956: <http://trac.webkit.org/changeset/60956>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.