Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1129123004:
Don't treat http://gpu as a WebUI url (Closed)
|
Created:
5 years, 7 months ago by Evan Stade Modified:
5 years, 7 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org, nasko+codewatch_chromium.org, creis+watch_chromium.org, ajwong+watch_chromium.org, James Hawkins Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't treat http://gpu as a WebUI url
BUG=464922
Committed: https://crrev.com/7cdcf50b4ae3858f5132a1bb0d4cb5e104527b42
Cr-Commit-Position: refs/heads/master@{#329285}
Patch Set 1 #Patch Set 2 : can't DCHECK #Patch Set 3 : rebase #Patch Set 4 : DEPS and another rebase #
Total comments: 5
Patch Set 5 : CONTENT_EXPORT #
Messages
Total messages: 26 (10 generated)
estade@chromium.org changed reviewers: + jam@chromium.org, wfh@chromium.org
the reason I modified existing unit tests instead of writing a new one for ContentWebUIControllerFactory is because I dislike tests that are overly simple (where the test basically looks identical to the function it's testing). I figure using the real factory where possible gives better overall coverage.
On 2015/05/07 17:02:42, Evan Stade wrote: > the reason I modified existing unit tests instead of writing a new one for > ContentWebUIControllerFactory is because I dislike tests that are overly simple > (where the test basically looks identical to the function it's testing). I > figure using the real factory where possible gives better overall coverage. I don't understand the linking errors --- everything links fine on my machine and the gyp dependencies seem to be in order.
estade@chromium.org changed reviewers: + davidben@chromium.org, felt@chromium.org
ping +more reviewers (felt and davidben) Also, tips on the link error?
On 2015/05/08 18:52:47, Evan Stade wrote: > ping > > +more reviewers (felt and davidben) > > Also, tips on the link error? I wonder if ContentWebUIControllerFactory in content/browser/webui/content_web_ui_controller_factory.h needs a CONTENT_EXPORT here.
lgtm https://codereview.chromium.org/1129123004/diff/60001/content/browser/DEPS File content/browser/DEPS (right): https://codereview.chromium.org/1129123004/diff/60001/content/browser/DEPS#ne... content/browser/DEPS:22: "+webui", Why is this line needed? (I don't even see a //webui.) https://codereview.chromium.org/1129123004/diff/60001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/1129123004/diff/60001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:230: ContentWebUIControllerFactory::GetInstance()); It does seem slightly off that we're pulling in the real WebUIControllerFactory. That brings in dependencies on GpuInternalsUI and AccessibilityUI and everything they depend on and blah. It's somewhat a lost cause what with how RenderViewHostImplTestHarness works at all, but I wonder if it's best to avoid pulling in so much of the stack lest the tests suddenly break when we add a sufficiently hefty WebUI. Then again, a unit test for the fix in ContentWebUIControllerFactory would have the same issue. So possibly this isn't worth caring too much over. I don't feel that strongly here. https://codereview.chromium.org/1129123004/diff/60001/content/browser/webui/c... File content/browser/webui/content_web_ui_controller_factory.cc (right): https://codereview.chromium.org/1129123004/diff/60001/content/browser/webui/c... content/browser/webui/content_web_ui_controller_factory.cc:82: ContentWebUIControllerFactory* ContentWebUIControllerFactory::GetInstance() { I believe the link issues you're having is because you need to CONTENT_EXPORT this class.
https://codereview.chromium.org/1129123004/diff/60001/content/browser/DEPS File content/browser/DEPS (right): https://codereview.chromium.org/1129123004/diff/60001/content/browser/DEPS#ne... content/browser/DEPS:22: "+webui", On 2015/05/09 00:31:05, David Benjamin wrote: > Why is this line needed? (I don't even see a //webui.) yea, I don't think it helps. It was supposed to refer to content/browser/webui --- it was a shot in the dark to fix the linking errors. https://codereview.chromium.org/1129123004/diff/60001/content/browser/webui/c... File content/browser/webui/content_web_ui_controller_factory.cc (right): https://codereview.chromium.org/1129123004/diff/60001/content/browser/webui/c... content/browser/webui/content_web_ui_controller_factory.cc:82: ContentWebUIControllerFactory* ContentWebUIControllerFactory::GetInstance() { On 2015/05/09 00:31:05, David Benjamin wrote: > I believe the link issues you're having is because you need to CONTENT_EXPORT > this class. will try that -- thanks!
the scheme check lgtm
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from felt@chromium.org, davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1129123004/#ps80001 (title: "CONTENT_EXPORT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129123004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL) win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL) win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129123004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_android on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129123004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129123004/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/7cdcf50b4ae3858f5132a1bb0d4cb5e104527b42 Cr-Commit-Position: refs/heads/master@{#329285} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
