Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2626863006:
[Chromecast] Add CastWebContents (Closed)
|
Created:
3 years, 11 months ago by derekjchow1 Modified:
3 years, 10 months ago CC:
chromium-reviews, alokp+watch_chromium.org, lcwu+watch_chromium.org, halliwell+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Chromecast] Add CastWebView
CastWebView is a simplified interface for loading and
displaying a WebContents.
BUG=internal b/33274359
TEST=cast_shell
Review-Url: https://codereview.chromium.org/2626863006
Cr-Commit-Position: refs/heads/master@{#452085}
Committed: https://chromium.googlesource.com/chromium/src/+/7348f0095cf19f8747e5e0dbdad4d91bc0fff0eb
Patch Set 1 #
Total comments: 2
Patch Set 2 : s/CastWebContents/CastWebView/g #
Total comments: 10
Patch Set 3 : [Chromecast] Add CastWebContents #
Total comments: 6
Patch Set 4 : [Chromecast] Add CastWebContents #Patch Set 5 : Rebase #Patch Set 6 : Browser Tests #Patch Set 7 : Long overdue rebase #
Total comments: 9
Patch Set 8 : [Chromecast] Add CastWebContents #Messages
Total messages: 51 (34 generated)
derekjchow@chromium.org changed reviewers: + alokp@chromium.org, halliwell@chromium.org, seantopping@chromium.org
As part of an effort to remove internal content dependencies, upstreaming a bit of our code to show how we use WebContents. PTAL
I like this change but have a high-level comment to make the boundary between this new class and CastContentWindow clearer. I will have more comments once I review the new class in more detail. https://codereview.chromium.org/2626863006/diff/1/chromecast/browser/cast_web... File chromecast/browser/cast_web_contents.cc (right): https://codereview.chromium.org/2626863006/diff/1/chromecast/browser/cast_web... chromecast/browser/cast_web_contents.cc:33: web_contents_(window_->CreateWebContents(browser_context_)), The overlap between CastContentWindow and CastWebContents becomes confusing to me. The ownership model is correct, but I would suggest the following to make things clearer: 1. Rename this class to CastWebView 2. Rename CastContentWindow to CastWindow, which should not have any dependency on WebContents. Move CreateWebContents functionality to this class.
https://codereview.chromium.org/2626863006/diff/1/chromecast/browser/cast_web... File chromecast/browser/cast_web_contents.cc (right): https://codereview.chromium.org/2626863006/diff/1/chromecast/browser/cast_web... chromecast/browser/cast_web_contents.cc:33: web_contents_(window_->CreateWebContents(browser_context_)), On 2017/01/14 00:43:40, alokp wrote: > The overlap between CastContentWindow and CastWebContents becomes confusing to > me. The ownership model is correct, but I would suggest the following to make > things clearer: > > 1. Rename this class to CastWebView Great idea. Done. > 2. Rename CastContentWindow to CastWindow, which should not have any dependency > on WebContents. Move CreateWebContents functionality to this class. Done to most of your comments. I haven't renamed CastContentWindow to CastWindow since Josh is working on the Window Manager. I'll rename at a later time. Also, we can't quite remove all dependencies on WebContents in CastWindow yet. Could we think that over in another CL?
Yes - we can rename it in another CL. Another round of comments on CastWebView API. https://codereview.chromium.org/2626863006/diff/20001/chromecast/browser/cast... File chromecast/browser/cast_web_view.h (right): https://codereview.chromium.org/2626863006/diff/20001/chromecast/browser/cast... chromecast/browser/cast_web_view.h:31: // or if the render process crashes. |error_code| will return a net error nit: "net error" -> net::Error https://codereview.chromium.org/2626863006/diff/20001/chromecast/browser/cast... chromecast/browser/cast_web_view.h:56: void LoadUrl(GURL url); Can this be called multiple times with a different URL? Do we have such a usecase? Can we pass the url in constructor if not? https://codereview.chromium.org/2626863006/diff/20001/chromecast/browser/cast... chromecast/browser/cast_web_view.h:59: void ClosePage(); Why do we need this API? Can't we just delete the instance of this class? https://codereview.chromium.org/2626863006/diff/20001/chromecast/browser/cast... chromecast/browser/cast_web_view.h:62: void MakeVisible(); nit: Show
The CQ bit was checked by derekjchow@chromium.org to run a CQ dry run
https://codereview.chromium.org/2626863006/diff/20001/chromecast/browser/cast... File chromecast/browser/cast_web_view.h (right): https://codereview.chromium.org/2626863006/diff/20001/chromecast/browser/cast... chromecast/browser/cast_web_view.h:31: // or if the render process crashes. |error_code| will return a net error On 2017/01/18 05:10:56, alokp wrote: > nit: "net error" -> net::Error Done. https://codereview.chromium.org/2626863006/diff/20001/chromecast/browser/cast... chromecast/browser/cast_web_view.h:56: void LoadUrl(GURL url); On 2017/01/18 05:10:56, alokp wrote: > Can this be called multiple times with a different URL? Do we have such a > usecase? Can we pass the url in constructor if not? Done. https://codereview.chromium.org/2626863006/diff/20001/chromecast/browser/cast... chromecast/browser/cast_web_view.h:59: void ClosePage(); On 2017/01/18 05:10:56, alokp wrote: > Why do we need this API? Can't we just delete the instance of this class? Calling ClosePage begins the closing process, which should trigger document.unload() and other clean up activities. A consumer of CastWebView can determine when the process has finished via OnPageStopped(). Deleting this class should force kill this page, which isn't always desirable. We need this API to distinguish between graceful and force shutdown. I've updated the comment to clarify purpose. https://codereview.chromium.org/2626863006/diff/20001/chromecast/browser/cast... chromecast/browser/cast_web_view.h:62: void MakeVisible(); On 2017/01/18 05:10:56, alokp wrote: > nit: Show Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2626863006/diff/20001/chromecast/browser/cast... File chromecast/browser/cast_web_view.h (right): https://codereview.chromium.org/2626863006/diff/20001/chromecast/browser/cast... chromecast/browser/cast_web_view.h:59: void ClosePage(); On 2017/01/19 01:22:11, derekjchow1 wrote: > On 2017/01/18 05:10:56, alokp wrote: > > Why do we need this API? Can't we just delete the instance of this class? > > Calling ClosePage begins the closing process, which should trigger > document.unload() and other clean up activities. A consumer of CastWebView can > determine when the process has finished via OnPageStopped(). > > Deleting this class should force kill this page, which isn't always desirable. > We need this API to distinguish between graceful and force shutdown. I've > updated the comment to clarify purpose. In that case this function should take a completion callback instead of an out-of-band notification on a client object. That said, I do not see the existing client waiting for the notification. Can we add it when we actually need it? https://codereview.chromium.org/2626863006/diff/40001/chromecast/browser/cast... File chromecast/browser/cast_web_view.cc (right): https://codereview.chromium.org/2626863006/diff/40001/chromecast/browser/cast... chromecast/browser/cast_web_view.cc:33: constexpr int kWebContentsDestructionDelayInMs = 50; this seems like a hack. How did you arrive at this number? https://codereview.chromium.org/2626863006/diff/40001/chromecast/browser/cast... chromecast/browser/cast_web_view.cc:95: void CastWebView::CloseContents(content::WebContents* source) { nit: please define functions in the same order as they are declared. Important: I do not see this function used anywhere. https://codereview.chromium.org/2626863006/diff/40001/chromecast/browser/cast... chromecast/browser/cast_web_view.cc:103: FROM_HERE, base::Bind(&CastWebView::DelayedCloseContents, Hmm. I do not understand this code. You do not start the unload here. How does just delaying the destruction of WebContents help?
https://codereview.chromium.org/2626863006/diff/40001/chromecast/browser/cast... File chromecast/browser/cast_web_view.cc (right): https://codereview.chromium.org/2626863006/diff/40001/chromecast/browser/cast... chromecast/browser/cast_web_view.cc:33: constexpr int kWebContentsDestructionDelayInMs = 50; On 2017/01/19 05:26:36, alokp wrote: > this seems like a hack. How did you arrive at this number? This is long-standing behaviour of Cast. Note that Chrome has a built-in timeout of 1 second for the unload handler to run: https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_vie... (if you write a simple page that loops forever in unload handler, you can see this time elapse when you close it). In our case, the next app takes priority, so we give old page a small window to run unload handling, but not much. We are reviewing whether it would be possible to give the closing page longer but running in background, but that has memory and CPU usage problems.
https://codereview.chromium.org/2626863006/diff/20001/chromecast/browser/cast... File chromecast/browser/cast_web_view.h (right): https://codereview.chromium.org/2626863006/diff/20001/chromecast/browser/cast... chromecast/browser/cast_web_view.h:59: void ClosePage(); On 2017/01/19 05:26:36, alokp wrote: > On 2017/01/19 01:22:11, derekjchow1 wrote: > > On 2017/01/18 05:10:56, alokp wrote: > > > Why do we need this API? Can't we just delete the instance of this class? > > > > Calling ClosePage begins the closing process, which should trigger > > document.unload() and other clean up activities. A consumer of CastWebView can > > determine when the process has finished via OnPageStopped(). > > > > Deleting this class should force kill this page, which isn't always desirable. > > We need this API to distinguish between graceful and force shutdown. I've > > updated the comment to clarify purpose. > > In that case this function should take a completion callback instead of an > out-of-band notification on a client object. > I disagree. We should handle page stops the same way, regardless if it was triggered by a ClosePage() call, the page closing itself or the render process dying. Having two APIs to know when the page stopped could leave to some confusion and corner cases (eg. "will the callback or delegate be called first?" or "what happens if the page crashes during document.onunload?"). Having a single API like OnPageStopped that provides a reason as to why the page closed is much simpler to understand and maintain. > That said, I do not see the existing client waiting for the notification. Can we > add it when we actually need it? See other comments. Although this isn't used publicly, this is required for internal use. https://codereview.chromium.org/2626863006/diff/40001/chromecast/browser/cast... File chromecast/browser/cast_web_view.cc (right): https://codereview.chromium.org/2626863006/diff/40001/chromecast/browser/cast... chromecast/browser/cast_web_view.cc:95: void CastWebView::CloseContents(content::WebContents* source) { On 2017/01/19 05:26:36, alokp wrote: > nit: please define functions in the same order as they are declared. > > Important: I do not see this function used anywhere. Done (regarding reordering). Although this function is not used publicly (since we never really stop a page in public cast_shell), it should be used internally (and is required to preserve existing behaviour). I do agree that we should use the function publicly. Would you like me to add a TODO? https://codereview.chromium.org/2626863006/diff/40001/chromecast/browser/cast... chromecast/browser/cast_web_view.cc:103: FROM_HERE, base::Bind(&CastWebView::DelayedCloseContents, On 2017/01/19 05:26:36, alokp wrote: > Hmm. I do not understand this code. You do not start the unload here. How does > just delaying the destruction of WebContents help? I added an additional comment. This function gets triggered after CastWebView::ClosePage() is called.
Ideally we should use all public APIs in public or at least have a test exercising those APIs, but I would not hold this patch for that reason. lgtm
The CQ bit was checked by derekjchow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
Will rebase and submit after Josh lands his change here: https://codereview.chromium.org/2643553002/
The CQ bit was checked by derekjchow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by derekjchow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
On 2017/01/20 01:11:46, derekjchow1 wrote: > Will rebase and submit after Josh lands his change here: > > https://codereview.chromium.org/2643553002/ Derek, do you want Sean to take this over? Josh's CL has landed now.
The CQ bit was checked by derekjchow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Sorry for the long delay, but I finally got the time to fix the browser test issues related to this patch. I looked internally, and it seems some WebApplication changes have been made as well. seantopping@, could you compare this with the internal class to see if there's more code we can upstream? Otherwise, I think this patch is good to go. I'd like alokp@ or halliwell@ to give another thumbs up though before I submit. https://codereview.chromium.org/2626863006/diff/120001/chromecast/browser/cas... File chromecast/browser/cast_web_view.h (right): https://codereview.chromium.org/2626863006/diff/120001/chromecast/browser/cas... chromecast/browser/cast_web_view.h:56: void LoadUrl(GURL url); Alok, I re-added this API (splitting it out of the constructor). It seems that for the browser tests, there are some steps that must be performed between WebContents construction and Starting a page. Similarly, for internal consumption we might want to add WebContentsObservers before calling LoadUrl to ensure on RenderFrameCreated is caught.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Looks good, just a couple small comments/questions. Btw, I think the CL subject + description needs updating (should say CastWebView now?) https://codereview.chromium.org/2626863006/diff/120001/chromecast/browser/cas... File chromecast/browser/cast_web_view.cc (right): https://codereview.chromium.org/2626863006/diff/120001/chromecast/browser/cas... chromecast/browser/cast_web_view.cc:90: CastWebView::~CastWebView() {} do we need WebContentsObserver::Observe(nullptr), or is that guaranteed to be taken care of elsewhere. https://codereview.chromium.org/2626863006/diff/120001/chromecast/browser/cas... chromecast/browser/cast_web_view.cc:105: // We need to delay the deletion of web_contents_ (currently for 50ms) to nit, delete 'currently for 50ms' ... just risks becoming incorrect :) https://codereview.chromium.org/2626863006/diff/120001/chromecast/browser/cas... chromecast/browser/cast_web_view.cc:141: if (source) { in what scenario is source null here? https://codereview.chromium.org/2626863006/diff/120001/chromecast/browser/ser... File chromecast/browser/service/cast_service_simple.cc (right): https://codereview.chromium.org/2626863006/diff/120001/chromecast/browser/ser... chromecast/browser/service/cast_service_simple.cc:62: base::MakeUnique<CastWebView>(this, browser_context(), nullptr, false); nit: comment what nullptr, false mean?
The CQ bit was checked by derekjchow@chromium.org to run a CQ dry run
Description was changed from ========== [Chromecast] Add CastWebContents CastWebContents is a simplified interface for loading and displaying a WebContents. BUG=internal b/33274359 TEST=cast_shell ========== to ========== [Chromecast] Add CastWebView CastWebView is a simplified interface for loading and displaying a WebContents. BUG=internal b/33274359 TEST=cast_shell ==========
Updated issue as well. Thanks for pointing that out Luke! https://codereview.chromium.org/2626863006/diff/120001/chromecast/browser/cas... File chromecast/browser/cast_web_view.cc (right): https://codereview.chromium.org/2626863006/diff/120001/chromecast/browser/cas... chromecast/browser/cast_web_view.cc:90: CastWebView::~CastWebView() {} On 2017/02/20 22:09:11, halliwell wrote: > do we need WebContentsObserver::Observe(nullptr), or is that guaranteed to be > taken care of elsewhere. Looks like it's taken care of in the WebContentsObserver destructor. https://cs.chromium.org/chromium/src/content/public/browser/web_contents_obse... https://codereview.chromium.org/2626863006/diff/120001/chromecast/browser/cas... chromecast/browser/cast_web_view.cc:105: // We need to delay the deletion of web_contents_ (currently for 50ms) to On 2017/02/20 22:09:11, halliwell wrote: > nit, delete 'currently for 50ms' ... just risks becoming incorrect :) Done. https://codereview.chromium.org/2626863006/diff/120001/chromecast/browser/cas... chromecast/browser/cast_web_view.cc:141: if (source) { On 2017/02/20 22:09:11, halliwell wrote: > in what scenario is source null here? I don't think it's ever null. I don't think web_contents_ can be null in this function since it's a WebContentsDelegate funciton. Removed check. https://codereview.chromium.org/2626863006/diff/120001/chromecast/browser/ser... File chromecast/browser/service/cast_service_simple.cc (right): https://codereview.chromium.org/2626863006/diff/120001/chromecast/browser/ser... chromecast/browser/service/cast_service_simple.cc:62: base::MakeUnique<CastWebView>(this, browser_context(), nullptr, false); On 2017/02/20 22:09:11, halliwell wrote: > nit: comment what nullptr, false mean? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/22 00:50:41, derekjchow1 wrote: > Updated issue as well. Thanks for pointing that out Luke! > > https://codereview.chromium.org/2626863006/diff/120001/chromecast/browser/cas... > File chromecast/browser/cast_web_view.cc (right): > > https://codereview.chromium.org/2626863006/diff/120001/chromecast/browser/cas... > chromecast/browser/cast_web_view.cc:90: CastWebView::~CastWebView() {} > On 2017/02/20 22:09:11, halliwell wrote: > > do we need WebContentsObserver::Observe(nullptr), or is that guaranteed to be > > taken care of elsewhere. > > Looks like it's taken care of in the WebContentsObserver destructor. > https://cs.chromium.org/chromium/src/content/public/browser/web_contents_obse... > > https://codereview.chromium.org/2626863006/diff/120001/chromecast/browser/cas... > chromecast/browser/cast_web_view.cc:105: // We need to delay the deletion of > web_contents_ (currently for 50ms) to > On 2017/02/20 22:09:11, halliwell wrote: > > nit, delete 'currently for 50ms' ... just risks becoming incorrect :) > > Done. > > https://codereview.chromium.org/2626863006/diff/120001/chromecast/browser/cas... > chromecast/browser/cast_web_view.cc:141: if (source) { > On 2017/02/20 22:09:11, halliwell wrote: > > in what scenario is source null here? > > I don't think it's ever null. I don't think web_contents_ can be null in this > function since it's a WebContentsDelegate funciton. Removed check. > > https://codereview.chromium.org/2626863006/diff/120001/chromecast/browser/ser... > File chromecast/browser/service/cast_service_simple.cc (right): > > https://codereview.chromium.org/2626863006/diff/120001/chromecast/browser/ser... > chromecast/browser/service/cast_service_simple.cc:62: > base::MakeUnique<CastWebView>(this, browser_context(), nullptr, false); > On 2017/02/20 22:09:11, halliwell wrote: > > nit: comment what nullptr, false mean? > > Done. lgtm, ninja
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by derekjchow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by derekjchow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alokp@chromium.org Link to the patchset: https://codereview.chromium.org/2626863006/#ps140001 (title: "[Chromecast] Add CastWebContents")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487781643326640,
"parent_rev": "cece2b21dec5a8f8695a0bf7743a63a8dae32224", "commit_rev":
"7348f0095cf19f8747e5e0dbdad4d91bc0fff0eb"}
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Add CastWebView CastWebView is a simplified interface for loading and displaying a WebContents. BUG=internal b/33274359 TEST=cast_shell ========== to ========== [Chromecast] Add CastWebView CastWebView is a simplified interface for loading and displaying a WebContents. BUG=internal b/33274359 TEST=cast_shell Review-Url: https://codereview.chromium.org/2626863006 Cr-Commit-Position: refs/heads/master@{#452085} Committed: https://chromium.googlesource.com/chromium/src/+/7348f0095cf19f8747e5e0dbdad4... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/7348f0095cf19f8747e5e0dbdad4... |
