Bug 54588

Created attachment 82695 [details] Patch

Comment on attachment 82695 [details] Patch R=me - let's let the EWs bots have a swing at this before cq+'ing just to be safe.

Attachment 82695 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7919484

Welp. Looks like upstream has added some more dependencies that didn't used to be there.

Created attachment 82702 [details] Patch

Attachment 82702 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7927356

Created attachment 82718 [details] Patch

Includes new libjingle dependency.

This makes me sad :( Why does DumpRenderTree need libjingle? What added this dependency?

+Albert No idea why DumpRenderTree is now dependent on libjingle. Albert - The only thing that depends on libjingle afaik is remoting. Any idea what would cause DumpRenderTree to now rely on it (changed sometime between now (Weds 5pm PST) and 30 hours ago (Tues, 11am PST).

(In reply to comment #10) > +Albert > > No idea why DumpRenderTree is now dependent on libjingle. > > Albert - The only thing that depends on libjingle afaik is remoting. Any idea what would cause DumpRenderTree to now rely on it (changed sometime between now (Weds 5pm PST) and 30 hours ago (Tues, 11am PST). +Sergey Actually, there's a few places that depend on libjingle (eg., sync). Also, we're starting to use it in PPAPI (albeit to support remoting and similar use cases, but technically its a separate feature). The CL that added the dependency is this: http://codereview.chromium.org/6478018 I think the larger question is if DRT needs to depend on PPAPI...it's just testing the webkit renderer right? Do we actually need pepper plugins for that?

(In reply to comment #11) > (In reply to comment #10) > > +Albert > > > > No idea why DumpRenderTree is now dependent on libjingle. > > > > Albert - The only thing that depends on libjingle afaik is remoting. Any idea what would cause DumpRenderTree to now rely on it (changed sometime between now (Weds 5pm PST) and 30 hours ago (Tues, 11am PST). > > +Sergey > > Actually, there's a few places that depend on libjingle (eg., sync). > > Also, we're starting to use it in PPAPI (albeit to support remoting and similar use cases, but technically its a separate feature). > > The CL that added the dependency is this: > http://codereview.chromium.org/6478018 > > I think the larger question is if DRT needs to depend on PPAPI...it's just testing the webkit renderer right? Do we actually need pepper plugins for that? Indeed. I think eventually we might want to run layout tests that test pepper plugins (we currently have layout tests for NPAPI plugins), but we don't currently have any such layout tests. It sucks that we keep having to add more and more (large) dependencies to this checkout. It increases the complexity of rolling, build+cycle times, and makes it harder to understand what's going on with the code.

(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > > +Albert > > > > > > No idea why DumpRenderTree is now dependent on libjingle. > > > > > > Albert - The only thing that depends on libjingle afaik is remoting. Any idea what would cause DumpRenderTree to now rely on it (changed sometime between now (Weds 5pm PST) and 30 hours ago (Tues, 11am PST). > > > > +Sergey > > > > Actually, there's a few places that depend on libjingle (eg., sync). > > > > Also, we're starting to use it in PPAPI (albeit to support remoting and similar use cases, but technically its a separate feature). > > > > The CL that added the dependency is this: > > http://codereview.chromium.org/6478018 > > > > I think the larger question is if DRT needs to depend on PPAPI...it's just testing the webkit renderer right? Do we actually need pepper plugins for that? > > Indeed. I think eventually we might want to run layout tests that test pepper plugins (we currently have layout tests for NPAPI plugins), but we don't currently have any such layout tests. > > It sucks that we keep having to add more and more (large) dependencies to this checkout. It increases the complexity of rolling, build+cycle times, and makes it harder to understand what's going on with the code. In future I plan to add an abstract interface for P2P, so it will be possible to mock it for layout tests, but I'm not sure if it worth it (do we need to test the full network stack in the layout tests?).

DRT should not need to depend on PPAPI. This dependency is just an artifact of the PPAPI implementation living in src/webkit/plugins/ppapi/ (within the chromium tree), and DRT depends on src/webkit. We should ideally break this dependency since it serves no value.

Could we break the dependency later? Would like to roll DEPS...

Comment on attachment 82718 [details] Patch Yes, of course. Can you please make sure that happens?

The commit-queue encountered the following flaky tests while processing attachment 82718 [details]: http/tests/security/cross-frame-access-port-explicit-domain.html bug 54668 (author: sam@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 82718 [details] Patch Clearing flags on attachment: 82718 Committed r78838: <http://trac.webkit.org/changeset/78838>

All reviewed patches have been landed. Closing bug.

(In reply to comment #16) > (From update of attachment 82718 [details]) > Yes, of course. Can you please make sure that happens? http://code.google.com/p/chromium/issues/detail?id=73304