Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1461693003:
[headless] Initial skeleton of headless/public/ (Closed)
Description[headless] Initial skeleton of headless/public/
Create outline of future Headless API.
BUG=546953
Committed: https://crrev.com/168fe2a9a59963bfd6a8d5b4762e316b3fbaf076
Cr-Commit-Position: refs/heads/master@{#362712}
Patch Set 1 #Patch Set 2 : Moved DEPS to correct directory #
Total comments: 37
Patch Set 3 : Changes according to Sami's notes #Patch Set 4 : Remove unneeded semicolons #
Total comments: 4
Patch Set 5 : Removed OnStart() and changed Run() #
Total comments: 1
Patch Set 6 : Pure virtual indeed #
Total comments: 13
Patch Set 7 : Changes according to alexclarke@'s comments #Patch Set 8 : Remove direct dom interaction #
Total comments: 55
Patch Set 9 : Changes according to skyostil@'s comments #
Total comments: 11
Patch Set 10 : Minor fixes #Patch Set 11 : Minor fixes #
Total comments: 6
Patch Set 12 : Even more minor fixes #Patch Set 13 : More minor fixes #
Total comments: 1
Messages
Total messages: 41 (12 generated)
skyostil@chromium.org changed reviewers: + skyostil@chromium.org
Thanks, looks like a good starting point. https://codereview.chromium.org/1461693003/diff/20001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/1461693003/diff/20001/headless/BUILD.gn#newco... headless/BUILD.gn:13: testonly = true The headless shell is testonly but the library itself should be a release artifact I think. https://codereview.chromium.org/1461693003/diff/20001/headless/public/headles... File headless/public/headless_browser.h (right): https://codereview.chromium.org/1461693003/diff/20001/headless/public/headles... headless/public/headless_browser.h:22: class HeadlessBrowser { I think we need a HEADLESS_EXPORT macro on all the classes here (see for example https://code.google.com/p/chromium/codesearch#chromium/src/components/schedul...) https://codereview.chromium.org/1461693003/diff/20001/headless/public/headles... headless/public/headless_browser.h:24: static HeadlessBrowser* Get(); Could you add some documentation comments for the methods here and in WebContents? The DOM parts will still change so let's document them later. https://codereview.chromium.org/1461693003/diff/20001/headless/public/headles... headless/public/headless_browser.h:26: virtual WebContents* CreateWebContents(int width, int height) = 0; Return a scoped_ptr here. gfx::Size instead of width/height? https://codereview.chromium.org/1461693003/diff/20001/headless/public/headles... headless/public/headless_browser.h:35: virtual void OnStart(const base::Closure& on_start_callback) = 0; Is this something the client would call? Maybe we can think of some better name for this since it's a little unclear what this does. https://codereview.chromium.org/1461693003/diff/20001/headless/public/headles... headless/public/headless_browser.h:37: virtual void TracingStartRecording( Probably best to leave tracing out for now since I'm not sure what the best shape for this API is yet. https://codereview.chromium.org/1461693003/diff/20001/headless/public/headles... headless/public/headless_browser.h:42: virtual ~HeadlessBrowser(){}; nit: space missing before {} (did you run git cl format?) https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_con... File headless/public/web_contents.h (right): https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_con... headless/public/web_contents.h:14: namespace gfx { Looks like this isn't needed. https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_con... headless/public/web_contents.h:18: namespace content { Ditto. https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_con... headless/public/web_contents.h:23: namespace aura { Ditto. https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_con... headless/public/web_contents.h:31: class WebContents { Some documentation also here would be nice. Especially about thread restrictions about the different methods. https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_con... headless/public/web_contents.h:44: scoped_ptr<ObserverImpl> observer_; Why do we need an impl pointer here? Seems like we could just have WebContents* as a member here -- or are you expecting to add something more complex? https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_con... headless/public/web_contents.h:50: typedef base::Callback<void(scoped_ptr<SkBitmap>)> ScreenshotCallback; nit: "using" is a little neater than typedef. https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_con... headless/public/web_contents.h:54: virtual ~WebContents(){}; nit: missing space before {} https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_ele... File headless/public/web_element.h (right): https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_ele... headless/public/web_element.h:27: // TODO(altimin): replace with headless::WebRect. This could also be a gfx::Rect. https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_ele... headless/public/web_element.h:29: const std::map<std::string, std::string>& Attributes() const; We'll probably want to turn this into an iterator but this is fine for now. https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_fra... File headless/public/web_frame.h (right): https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_fra... headless/public/web_frame.h:22: virtual void ExecuteScript(const std::string& source_code) = 0; Could you add a TODO for figuring out a way to return a result here?
PTAL https://codereview.chromium.org/1461693003/diff/20001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/1461693003/diff/20001/headless/BUILD.gn#newco... headless/BUILD.gn:13: testonly = true On 2015/11/19 14:08:34, Sami wrote: > The headless shell is testonly but the library itself should be a release > artifact I think. Done. https://codereview.chromium.org/1461693003/diff/20001/headless/public/headles... File headless/public/headless_browser.h (right): https://codereview.chromium.org/1461693003/diff/20001/headless/public/headles... headless/public/headless_browser.h:22: class HeadlessBrowser { On 2015/11/19 14:08:34, Sami wrote: > I think we need a HEADLESS_EXPORT macro on all the classes here (see for example > https://code.google.com/p/chromium/codesearch#chromium/src/components/schedul...) Done. https://codereview.chromium.org/1461693003/diff/20001/headless/public/headles... headless/public/headless_browser.h:24: static HeadlessBrowser* Get(); On 2015/11/19 14:08:34, Sami wrote: > Could you add some documentation comments for the methods here and in > WebContents? The DOM parts will still change so let's document them later. Done. https://codereview.chromium.org/1461693003/diff/20001/headless/public/headles... headless/public/headless_browser.h:26: virtual WebContents* CreateWebContents(int width, int height) = 0; On 2015/11/19 14:08:34, Sami wrote: > Return a scoped_ptr here. gfx::Size instead of width/height? Done. https://codereview.chromium.org/1461693003/diff/20001/headless/public/headles... headless/public/headless_browser.h:35: virtual void OnStart(const base::Closure& on_start_callback) = 0; On 2015/11/19 14:08:34, Sami wrote: > Is this something the client would call? Maybe we can think of some better name > for this since it's a little unclear what this does. We want to give user an ability to do some work as browser starts (open pages etc). This is the function to register callback to be executed as soon as browser starts. https://codereview.chromium.org/1461693003/diff/20001/headless/public/headles... headless/public/headless_browser.h:37: virtual void TracingStartRecording( On 2015/11/19 14:08:34, Sami wrote: > Probably best to leave tracing out for now since I'm not sure what the best > shape for this API is yet. Done. https://codereview.chromium.org/1461693003/diff/20001/headless/public/headles... headless/public/headless_browser.h:42: virtual ~HeadlessBrowser(){}; On 2015/11/19 14:08:34, Sami wrote: > nit: space missing before {} (did you run git cl format?) Yes! And git cl format does exactly this. https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_con... File headless/public/web_contents.h (right): https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_con... headless/public/web_contents.h:14: namespace gfx { On 2015/11/19 14:08:34, Sami wrote: > Looks like this isn't needed. Done. https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_con... headless/public/web_contents.h:18: namespace content { On 2015/11/19 14:08:34, Sami wrote: > Ditto. Done. https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_con... headless/public/web_contents.h:23: namespace aura { On 2015/11/19 14:08:34, Sami wrote: > Ditto. Done. https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_con... headless/public/web_contents.h:31: class WebContents { On 2015/11/19 14:08:34, Sami wrote: > Some documentation also here would be nice. Especially about thread restrictions > about the different methods. Done. https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_con... headless/public/web_contents.h:44: scoped_ptr<ObserverImpl> observer_; On 2015/11/19 14:08:34, Sami wrote: > Why do we need an impl pointer here? Seems like we could just have WebContents* > as a member here -- or are you expecting to add something more complex? Yes, we need to have content::WebContentsObserver inside. https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_con... headless/public/web_contents.h:50: typedef base::Callback<void(scoped_ptr<SkBitmap>)> ScreenshotCallback; On 2015/11/19 14:08:34, Sami wrote: > nit: "using" is a little neater than typedef. Done. https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_con... headless/public/web_contents.h:54: virtual ~WebContents(){}; On 2015/11/19 14:08:34, Sami wrote: > nit: missing space before {} It's git cl format behaviour. https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_ele... File headless/public/web_element.h (right): https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_ele... headless/public/web_element.h:27: // TODO(altimin): replace with headless::WebRect. On 2015/11/19 14:08:35, Sami wrote: > This could also be a gfx::Rect. Done. https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_ele... headless/public/web_element.h:29: const std::map<std::string, std::string>& Attributes() const; On 2015/11/19 14:08:34, Sami wrote: > We'll probably want to turn this into an iterator but this is fine for now. But we also want to access individual attributes: element.Attributes()["attribute_name"]. https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_fra... File headless/public/web_frame.h (right): https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_fra... headless/public/web_frame.h:22: virtual void ExecuteScript(const std::string& source_code) = 0; On 2015/11/19 14:08:35, Sami wrote: > Could you add a TODO for figuring out a way to return a result here? I've already found a way to do so (see the cl with demo), but we will need to add v8 to DEPS, and I'd prefer to do it later.
Description was changed from ========== [headless] Initial skeleton of headless/public/ Create outline of future Headless API. BUG=546953 ========== to ========== [headless] Initial skeleton of headless/public/ Create outline of future Headless API. BUG=546953 ==========
altimin@chromium.org changed reviewers: + alexclarke@chromium.org
Thanks, couple more nits and a suggestion for OnStart(). https://codereview.chromium.org/1461693003/diff/20001/headless/public/headles... File headless/public/headless_browser.h (right): https://codereview.chromium.org/1461693003/diff/20001/headless/public/headles... headless/public/headless_browser.h:35: virtual void OnStart(const base::Closure& on_start_callback) = 0; On 2015/11/19 14:52:54, altimin wrote: > On 2015/11/19 14:08:34, Sami wrote: > > Is this something the client would call? Maybe we can think of some better > name > > for this since it's a little unclear what this does. > > We want to give user an ability to do some work as browser starts (open pages > etc). This is the function to register callback to be executed as soon as > browser starts. Should we just have the user pass this into Run? Seems like you would always want to use this. https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_ele... File headless/public/web_element.h (right): https://codereview.chromium.org/1461693003/diff/20001/headless/public/web_ele... headless/public/web_element.h:29: const std::map<std::string, std::string>& Attributes() const; On 2015/11/19 14:52:54, altimin wrote: > On 2015/11/19 14:08:34, Sami wrote: > > We'll probably want to turn this into an iterator but this is fine for now. > > But we also want to access individual attributes: > element.Attributes()["attribute_name"]. Right, the problem is that making this a map requires us to make copies of all the attributes. With the iterator the user can make their own maps (or we could have a helper for that). Anyway, let's leave this as is for now. https://codereview.chromium.org/1461693003/diff/60001/headless/public/headles... File headless/public/headless_browser.h (right): https://codereview.chromium.org/1461693003/diff/60001/headless/public/headles... headless/public/headless_browser.h:16: namespace trace_event { nit: remove https://codereview.chromium.org/1461693003/diff/60001/headless/public/web_con... File headless/public/web_contents.h (right): https://codereview.chromium.org/1461693003/diff/60001/headless/public/web_con... headless/public/web_contents.h:39: // Requests browser tab to open an url. nit: s/open/navigate to/
PTAL https://codereview.chromium.org/1461693003/diff/20001/headless/public/headles... File headless/public/headless_browser.h (right): https://codereview.chromium.org/1461693003/diff/20001/headless/public/headles... headless/public/headless_browser.h:35: virtual void OnStart(const base::Closure& on_start_callback) = 0; On 2015/11/19 15:42:44, Sami wrote: > On 2015/11/19 14:52:54, altimin wrote: > > On 2015/11/19 14:08:34, Sami wrote: > > > Is this something the client would call? Maybe we can think of some better > > name > > > for this since it's a little unclear what this does. > > > > We want to give user an ability to do some work as browser starts (open pages > > etc). This is the function to register callback to be executed as soon as > > browser starts. > > Should we just have the user pass this into Run? Seems like you would always > want to use this. Done. https://codereview.chromium.org/1461693003/diff/60001/headless/public/headles... File headless/public/headless_browser.h (right): https://codereview.chromium.org/1461693003/diff/60001/headless/public/headles... headless/public/headless_browser.h:16: namespace trace_event { On 2015/11/19 15:42:44, Sami wrote: > nit: remove Done. https://codereview.chromium.org/1461693003/diff/60001/headless/public/web_con... File headless/public/web_contents.h (right): https://codereview.chromium.org/1461693003/diff/60001/headless/public/web_con... headless/public/web_contents.h:39: // Requests browser tab to open an url. On 2015/11/19 15:42:44, Sami wrote: > nit: s/open/navigate to/ Done.
lgtm with a nit. Dimitri, could you check the DEPS change here please? https://codereview.chromium.org/1461693003/diff/80001/headless/public/headles... File headless/public/headless_browser.h (right): https://codereview.chromium.org/1461693003/diff/80001/headless/public/headles... headless/public/headless_browser.h:45: const base::Closure& on_browser_start_callback); Pure virtual.
On 2015/11/19 16:24:39, Sami wrote: > Dimitri, could you check the DEPS change here please? And obviously the overall sanity of this approach :)
skyostil@chromium.org changed reviewers: + dglazkov@chromium.org
+dglazkov@ for real this time.
https://codereview.chromium.org/1461693003/diff/100001/headless/public/headle... File headless/public/headless_browser.h (right): https://codereview.chromium.org/1461693003/diff/100001/headless/public/headle... headless/public/headless_browser.h:29: virtual scoped_ptr<WebContents> CreateWebContents(const gfx::Size& size) = 0; Is the idea that inside of the closure passed to Run() we'd call HeadlessBrowser::Get()->CreateWebContents()? If so it might make sense to sketch this out in the comment block below. https://codereview.chromium.org/1461693003/diff/100001/headless/public/web_co... File headless/public/web_contents.h (right): https://codereview.chromium.org/1461693003/diff/100001/headless/public/web_co... headless/public/web_contents.h:27: Observer(WebContents* web_contents); I note we are not providing the implementation in this patch, was that intended? nit: explicit ultra nit: Should this take a scoped_ptr<ObserverImpl> instead of a naked pointer? That way it's obvious it takes ownership. https://codereview.chromium.org/1461693003/diff/100001/headless/public/web_co... headless/public/web_contents.h:41: virtual void OpenURL(const GURL& url); This won't always succeed, how are we going to report errors? https://codereview.chromium.org/1461693003/diff/100001/headless/public/web_do... File headless/public/web_document.h (right): https://codereview.chromium.org/1461693003/diff/100001/headless/public/web_do... headless/public/web_document.h:19: WebDocument(const blink::WebDocument& web_document); nit: explicit https://codereview.chromium.org/1461693003/diff/100001/headless/public/web_el... File headless/public/web_element.h (right): https://codereview.chromium.org/1461693003/diff/100001/headless/public/web_el... headless/public/web_element.h:26: WebElement(const blink::WebElement& web_element); nit: explicit for both of these https://codereview.chromium.org/1461693003/diff/100001/headless/public/web_fr... File headless/public/web_frame.h (right): https://codereview.chromium.org/1461693003/diff/100001/headless/public/web_fr... headless/public/web_frame.h:19: virtual ~WebFrame(); Can we not implement this destructor inline?
PTAL https://codereview.chromium.org/1461693003/diff/100001/headless/public/headle... File headless/public/headless_browser.h (right): https://codereview.chromium.org/1461693003/diff/100001/headless/public/headle... headless/public/headless_browser.h:29: virtual scoped_ptr<WebContents> CreateWebContents(const gfx::Size& size) = 0; On 2015/11/19 18:17:54, alexclarke1 wrote: > Is the idea that inside of the closure passed to Run() we'd call > HeadlessBrowser::Get()->CreateWebContents()? > > If so it might make sense to sketch this out in the comment block below. That's one of the uses, yes, but it's not the only one. For example, in the scenario when we will want to load several web pages in order, we will end calling CreateWebContents() from Observer::OnDocumentLoadCompleted(). https://codereview.chromium.org/1461693003/diff/100001/headless/public/web_co... File headless/public/web_contents.h (right): https://codereview.chromium.org/1461693003/diff/100001/headless/public/web_co... headless/public/web_contents.h:27: Observer(WebContents* web_contents); On 2015/11/19 18:17:54, alexclarke1 wrote: > I note we are not providing the implementation in this patch, was that intended? > > nit: explicit > ultra nit: Should this take a scoped_ptr<ObserverImpl> instead of a naked > pointer? That way it's obvious it takes ownership. Yes, it is. I didn't really understand the bit about scoped_ptr<ObserverImpl>. https://codereview.chromium.org/1461693003/diff/100001/headless/public/web_co... headless/public/web_contents.h:41: virtual void OpenURL(const GURL& url); On 2015/11/19 18:17:54, alexclarke1 wrote: > This won't always succeed, how are we going to report errors? In bright and not so distant future observer will have more methods, not just DocumentOnLoadCompleted(). https://codereview.chromium.org/1461693003/diff/100001/headless/public/web_do... File headless/public/web_document.h (right): https://codereview.chromium.org/1461693003/diff/100001/headless/public/web_do... headless/public/web_document.h:19: WebDocument(const blink::WebDocument& web_document); On 2015/11/19 18:17:54, alexclarke1 wrote: > nit: explicit Done. https://codereview.chromium.org/1461693003/diff/100001/headless/public/web_el... File headless/public/web_element.h (right): https://codereview.chromium.org/1461693003/diff/100001/headless/public/web_el... headless/public/web_element.h:26: WebElement(const blink::WebElement& web_element); On 2015/11/19 18:17:54, alexclarke1 wrote: > nit: explicit for both of these Done. https://codereview.chromium.org/1461693003/diff/100001/headless/public/web_fr... File headless/public/web_frame.h (right): https://codereview.chromium.org/1461693003/diff/100001/headless/public/web_fr... headless/public/web_frame.h:19: virtual ~WebFrame(); On 2015/11/19 18:17:54, alexclarke1 wrote: > Can we not implement this destructor inline? Done.
altimin@chromium.org changed reviewers: - dglazkov@chromium.org
PTAL
Thanks for leaving out the DOM parts for now. Generally looks good -- some bikesheddy comments. https://codereview.chromium.org/1461693003/diff/140001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/1461693003/diff/140001/headless/BUILD.gn#newc... headless/BUILD.gn:1: # Use of this source code is governed by a BSD-style license that can be Missing the first line here? https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... File headless/public/headless_browser.cc (right): https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.cc:15: : argc(std::move(options.argc)), I think you can just do a *this = std::move(options); https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.cc:45: return std::move(options_); No need for std::move here -- returning already makes the result an rvalue. https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... File headless/public/headless_browser.h (right): https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.h:39: virtual scoped_ptr<WebContents> CreateWebContents(const gfx::Size& size) = 0; Which of these methods are safe to call before entering run? I wonder if we should make our lives simpler by making Run be the static entrypoint and passing the HeadlessBrowser pointer to the on_browser_start closure? https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.h:50: // argc, argv, base::Bind(function_to_be_run_on_browser_start)); This comment looks out of date. We could move the example code to the toplevel README.md file. https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.h:55: // Requests browser to stop as soon as possible. Might want to mention that this causes |Run| to return. https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.h:59: const base::Closure& tracing_started) = 0; Let's name callbacks consistently: either everything with an "on_" prefix or without one. https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.h:64: virtual ~HeadlessBrowser() {} DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.h:68: Options(Options&&); Missing a move assignment operator. https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.h:77: scoped_ptr<std::string> user_agent; Can we make this a regular string that's initialized to the default user agent? https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.h:79: // If not null, create start devtools for remote debugging A heap allocation seems overkill here. Could we make this kInvalidPort (-1) by default and then only act on it if its non-negative? https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.h:83: // Using URLRequestContextGetter we can provide our own implementation of nit: instead of talking about 'we' (who?) how about something like Optional URLRequestContextGetter for customizing the network stack. https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.h:89: Options(int argc, const char** argv); DISALLOW_COPY_AND_ASSIGN? Not sure how that interacts with the move constructor. https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.h:98: Builder& EnableDevtoolsServer(int port); s/Devtools/DevTools/ https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.h:106: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1461693003/diff/140001/headless/public/network.h File headless/public/network.h (right): https://codereview.chromium.org/1461693003/diff/140001/headless/public/networ... headless/public/network.h:19: scoped_refptr<net::URLRequestContextGetter> Please define these as static methods in a Network class instead of naked functions. That way they will be easier to find in documentation. https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_co... File headless/public/web_contents.h (right): https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_co... headless/public/web_contents.h:22: class HEADLESS_EXPORT WebContents { Could just have a top-level comment here that says what this is and that it can only be used on the browser main thread (so that every method doesn't need to mention it specifically). https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_co... headless/public/web_contents.h:26: class Observer { How do you set the observer? Can you have several? https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_co... headless/public/web_contents.h:39: scoped_ptr<ObserverImpl> observer_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_co... headless/public/web_contents.h:44: virtual scoped_ptr<WebFrame> main_frame() = 0; I don't think we can return a scoped_ptr here. That would mean the caller takes ownership. Also, please don't use hacker_style() names on virtual functions. https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_co... headless/public/web_contents.h:56: virtual content::WebContents* web_contents() = 0; Does this need to be here? I imagine the impl classes would just deal with concrete impl types without going through the virtual base class? https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_co... headless/public/web_contents.h:57: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_fr... File headless/public/web_frame.h (right): https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_fr... headless/public/web_frame.h:16: class HEADLESS_EXPORT WebFrame { nit: Add a description and say that this also belongs to the renderer main thread. https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_fr... headless/public/web_frame.h:23: // Schedule given script for execution. Maybe: Execute the given script, ignoring its return value. Assuming we can remove the callback below, maybe we should drop this function completely and always return the value? https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_fr... headless/public/web_frame.h:26: // Execute given script and return value. s/value/its result/. https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_fr... headless/public/web_frame.h:40: virtual void ExecuteScriptAndReturnValue( Since this is all running on the renderer main thread, I think we can just return the result directly without a callback.
PTAL https://codereview.chromium.org/1461693003/diff/140001/headless/BUILD.gn File headless/BUILD.gn (right): https://codereview.chromium.org/1461693003/diff/140001/headless/BUILD.gn#newc... headless/BUILD.gn:1: # Use of this source code is governed by a BSD-style license that can be On 2015/12/01 14:03:57, Sami wrote: > Missing the first line here? Done. https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... File headless/public/headless_browser.cc (right): https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.cc:15: : argc(std::move(options.argc)), On 2015/12/01 14:03:57, Sami wrote: > I think you can just do a *this = std::move(options); Done. https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.cc:45: return std::move(options_); On 2015/12/01 14:03:57, Sami wrote: > No need for std::move here -- returning already makes the result an rvalue. No. Returning makes only temporary variables rvalue, this doesn't happen with class members. https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... File headless/public/headless_browser.h (right): https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.h:39: virtual scoped_ptr<WebContents> CreateWebContents(const gfx::Size& size) = 0; On 2015/12/01 14:03:57, Sami wrote: > Which of these methods are safe to call before entering run? I wonder if we > should make our lives simpler by making Run be the static entrypoint and passing > the HeadlessBrowser pointer to the on_browser_start closure? Great idea. Done. https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.h:50: // argc, argv, base::Bind(function_to_be_run_on_browser_start)); On 2015/12/01 14:03:57, Sami wrote: > This comment looks out of date. We could move the example code to the toplevel > README.md file. Done. https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.h:55: // Requests browser to stop as soon as possible. On 2015/12/01 14:03:57, Sami wrote: > Might want to mention that this causes |Run| to return. Done. https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.h:59: const base::Closure& tracing_started) = 0; On 2015/12/01 14:03:57, Sami wrote: > Let's name callbacks consistently: either everything with an "on_" prefix or > without one. Done. https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.h:64: virtual ~HeadlessBrowser() {} On 2015/12/01 14:03:57, Sami wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.h:68: Options(Options&&); On 2015/12/01 14:03:57, Sami wrote: > Missing a move assignment operator. Done. https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.h:77: scoped_ptr<std::string> user_agent; On 2015/12/01 14:03:57, Sami wrote: > Can we make this a regular string that's initialized to the default user agent? Done. https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.h:79: // If not null, create start devtools for remote debugging On 2015/12/01 14:03:57, Sami wrote: > A heap allocation seems overkill here. Could we make this kInvalidPort (-1) by > default and then only act on it if its non-negative? Done. https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.h:83: // Using URLRequestContextGetter we can provide our own implementation of On 2015/12/01 14:03:57, Sami wrote: > nit: instead of talking about 'we' (who?) how about something like > > Optional URLRequestContextGetter for customizing the network stack. Done. https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.h:89: Options(int argc, const char** argv); On 2015/12/01 14:03:57, Sami wrote: > DISALLOW_COPY_AND_ASSIGN? Not sure how that interacts with the move constructor. Done. https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.h:98: Builder& EnableDevtoolsServer(int port); On 2015/12/01 14:03:57, Sami wrote: > s/Devtools/DevTools/ Done. https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.h:106: }; On 2015/12/01 14:03:57, Sami wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1461693003/diff/140001/headless/public/network.h File headless/public/network.h (right): https://codereview.chromium.org/1461693003/diff/140001/headless/public/networ... headless/public/network.h:19: scoped_refptr<net::URLRequestContextGetter> On 2015/12/01 14:03:57, Sami wrote: > Please define these as static methods in a Network class instead of naked > functions. That way they will be easier to find in documentation. Done. https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_co... File headless/public/web_contents.h (right): https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_co... headless/public/web_contents.h:22: class HEADLESS_EXPORT WebContents { On 2015/12/01 14:03:58, Sami wrote: > Could just have a top-level comment here that says what this is and that it can > only be used on the browser main thread (so that every method doesn't need to > mention it specifically). Done. https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_co... headless/public/web_contents.h:39: scoped_ptr<ObserverImpl> observer_; On 2015/12/01 14:03:58, Sami wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_co... headless/public/web_contents.h:44: virtual scoped_ptr<WebFrame> main_frame() = 0; On 2015/12/01 14:03:58, Sami wrote: > I don't think we can return a scoped_ptr here. That would mean the caller takes > ownership. Also, please don't use hacker_style() names on virtual functions. Done. https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_co... headless/public/web_contents.h:56: virtual content::WebContents* web_contents() = 0; On 2015/12/01 14:03:57, Sami wrote: > Does this need to be here? I imagine the impl classes would just deal with > concrete impl types without going through the virtual base class? This is needed for Observer (which is a wrapper around content::WebContentsObserver, which require content::WebContents to be passed to constructor). Alternatively, we can cast WebContents to WebContentsImpl in Observer, but I wonder if it is going to be a good idea. https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_fr... File headless/public/web_frame.h (right): https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_fr... headless/public/web_frame.h:16: class HEADLESS_EXPORT WebFrame { On 2015/12/01 14:03:58, Sami wrote: > nit: Add a description and say that this also belongs to the renderer main > thread. Done. https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_fr... headless/public/web_frame.h:23: // Schedule given script for execution. On 2015/12/01 14:03:58, Sami wrote: > Maybe: Execute the given script, ignoring its return value. > > Assuming we can remove the callback below, maybe we should drop this function > completely and always return the value? There are two different methods in underlying Blink API. Looks like that the version without returning value is more efficient. https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_fr... headless/public/web_frame.h:26: // Execute given script and return value. On 2015/12/01 14:03:58, Sami wrote: > s/value/its result/. Done. https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_fr... headless/public/web_frame.h:40: virtual void ExecuteScriptAndReturnValue( On 2015/12/01 14:03:58, Sami wrote: > Since this is all running on the renderer main thread, I think we can just > return the result directly without a callback. No, we can't. Underlying API returns a callback so we are stuck with callbacks.
https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... File headless/public/headless_browser.cc (right): https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.cc:45: return std::move(options_); On 2015/12/01 15:17:28, altimin wrote: > On 2015/12/01 14:03:57, Sami wrote: > > No need for std::move here -- returning already makes the result an rvalue. > > No. Returning makes only temporary variables rvalue, this doesn't happen with > class members. Ah, right, because we shouldn't move those members out of the class. In this case the Builder should always be thrown away after calling Build() so it should be fine to move Options out of it, but I think it's more confusing than useful so let's just keep the simple code that you have now. https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_fr... File headless/public/web_frame.h (right): https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_fr... headless/public/web_frame.h:40: virtual void ExecuteScriptAndReturnValue( On 2015/12/01 15:17:29, altimin wrote: > On 2015/12/01 14:03:58, Sami wrote: > > Since this is all running on the renderer main thread, I think we can just > > return the result directly without a callback. > > No, we can't. Underlying API returns a callback so we are stuck with callbacks. Are you sure? This one looks synchronous: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/1461693003/diff/160001/headless/public/headle... File headless/public/headless_browser.cc (right): https://codereview.chromium.org/1461693003/diff/160001/headless/public/headle... headless/public/headless_browser.cc:16: *this = options; nit: the documentation seems to suggest that we do need the std::move() here: https://www.chromium.org/rvalue-references#TOC-9.-Move-constructors-should-be.... https://codereview.chromium.org/1461693003/diff/160001/headless/public/headle... headless/public/headless_browser.cc:43: // return std::move(options_); Bad edit? https://codereview.chromium.org/1461693003/diff/160001/headless/public/network.h File headless/public/network.h (right): https://codereview.chromium.org/1461693003/diff/160001/headless/public/networ... headless/public/network.h:28: }; nit: delete the constructor? https://codereview.chromium.org/1461693003/diff/160001/headless/public/web_co... File headless/public/web_contents.h (right): https://codereview.chromium.org/1461693003/diff/160001/headless/public/web_co... headless/public/web_contents.h:48: // Should be called on browser main thread. Does this need to be on the renderer thread instead? I wonder how we could make this more obvious...
https://codereview.chromium.org/1461693003/diff/160001/headless/public/headle... File headless/public/headless_browser.cc (right): https://codereview.chromium.org/1461693003/diff/160001/headless/public/headle... headless/public/headless_browser.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. I think this file should be headless/src/? (it used to be lib but turns out src is more correct.)
PTAL https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_fr... File headless/public/web_frame.h (right): https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_fr... headless/public/web_frame.h:40: virtual void ExecuteScriptAndReturnValue( On 2015/12/01 17:47:33, Sami wrote: > On 2015/12/01 15:17:29, altimin wrote: > > On 2015/12/01 14:03:58, Sami wrote: > > > Since this is all running on the renderer main thread, I think we can just > > > return the result directly without a callback. > > > > No, we can't. Underlying API returns a callback so we are stuck with > callbacks. > > Are you sure? This one looks synchronous: > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... But it is deprecated and async version should be used. https://codereview.chromium.org/1461693003/diff/160001/headless/public/headle... File headless/public/headless_browser.cc (right): https://codereview.chromium.org/1461693003/diff/160001/headless/public/headle... headless/public/headless_browser.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/12/01 17:48:27, Sami wrote: > I think this file should be headless/src/? (it used to be lib but turns out src > is more correct.) I followed example of content/public/, where public directory contains headers and occasional .cc files and other directories contain implementations. https://codereview.chromium.org/1461693003/diff/160001/headless/public/headle... headless/public/headless_browser.cc:16: *this = options; On 2015/12/01 17:47:33, Sami wrote: > nit: the documentation seems to suggest that we do need the std::move() here: > > https://www.chromium.org/rvalue-references#TOC-9.-Move-constructors-should-be.... Done. https://codereview.chromium.org/1461693003/diff/160001/headless/public/headle... headless/public/headless_browser.cc:43: // return std::move(options_); On 2015/12/01 17:47:33, Sami wrote: > Bad edit? Indeed, thanks. https://codereview.chromium.org/1461693003/diff/160001/headless/public/network.h File headless/public/network.h (right): https://codereview.chromium.org/1461693003/diff/160001/headless/public/networ... headless/public/network.h:28: }; On 2015/12/01 17:47:33, Sami wrote: > nit: delete the constructor? Done. https://codereview.chromium.org/1461693003/diff/160001/headless/public/web_co... File headless/public/web_contents.h (right): https://codereview.chromium.org/1461693003/diff/160001/headless/public/web_co... headless/public/web_contents.h:48: // Should be called on browser main thread. On 2015/12/01 17:47:34, Sami wrote: > Does this need to be on the renderer thread instead? I wonder how we could make > this more obvious... Oh, my bad. Yes, renderer thread indeed, thanks!
https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_fr... File headless/public/web_frame.h (right): https://codereview.chromium.org/1461693003/diff/140001/headless/public/web_fr... headless/public/web_frame.h:40: virtual void ExecuteScriptAndReturnValue( On 2015/12/01 18:42:46, altimin wrote: > On 2015/12/01 17:47:33, Sami wrote: > > On 2015/12/01 15:17:29, altimin wrote: > > > On 2015/12/01 14:03:58, Sami wrote: > > > > Since this is all running on the renderer main thread, I think we can just > > > > return the result directly without a callback. > > > > > > No, we can't. Underlying API returns a callback so we are stuck with > > callbacks. > > > > Are you sure? This one looks synchronous: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > But it is deprecated and async version should be used. Ah, I missed that. This is fine then. https://codereview.chromium.org/1461693003/diff/160001/headless/public/headle... File headless/public/headless_browser.cc (right): https://codereview.chromium.org/1461693003/diff/160001/headless/public/headle... headless/public/headless_browser.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/12/01 18:42:46, altimin wrote: > On 2015/12/01 17:48:27, Sami wrote: > > I think this file should be headless/src/? (it used to be lib but turns out > src > > is more correct.) > > I followed example of content/public/, where public directory contains headers > and occasional .cc files and other directories contain implementations. I see. I wonder where we should draw the line. For a user of the headless library it would be very confusing to me to see any .cc files in this directory, so maybe this is a bit of a special case.
https://codereview.chromium.org/1461693003/diff/100001/headless/public/web_co... File headless/public/web_contents.h (right): https://codereview.chromium.org/1461693003/diff/100001/headless/public/web_co... headless/public/web_contents.h:27: Observer(WebContents* web_contents); On 2015/11/19 18:36:24, altimin wrote: > On 2015/11/19 18:17:54, alexclarke1 wrote: > > I note we are not providing the implementation in this patch, was that > intended? > > > > nit: explicit > > ultra nit: Should this take a scoped_ptr<ObserverImpl> instead of a naked > > pointer? That way it's obvious it takes ownership. > > Yes, it is. > > I didn't really understand the bit about scoped_ptr<ObserverImpl>. I was assuming it took ownership of |web_contents|, but you probably don't want to do that :) https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... File headless/public/headless_browser.cc (right): https://codereview.chromium.org/1461693003/diff/140001/headless/public/headle... headless/public/headless_browser.cc:45: return std::move(options_); On 2015/12/01 17:47:33, Sami wrote: > On 2015/12/01 15:17:28, altimin wrote: > > On 2015/12/01 14:03:57, Sami wrote: > > > No need for std::move here -- returning already makes the result an rvalue. > > > > No. Returning makes only temporary variables rvalue, this doesn't happen with > > class members. > > Ah, right, because we shouldn't move those members out of the class. In this > case the Builder should always be thrown away after calling Build() so it should > be fine to move Options out of it, but I think it's more confusing than useful > so let's just keep the simple code that you have now. I'd argue we just don't need to care about the performance of an outer class like this. If this was hot inner code then sure by all means use std::move. :) https://codereview.chromium.org/1461693003/diff/200001/headless/public/headle... File headless/public/headless_browser.h (right): https://codereview.chromium.org/1461693003/diff/200001/headless/public/headle... headless/public/headless_browser.h:68: Options(Options&&); Do we actually need an explicit move and assignment constructor? Particularly for a class like this which will probably be created once? https://codereview.chromium.org/1461693003/diff/200001/headless/public/headle... headless/public/headless_browser.h:91: DISALLOW_COPY_AND_ASSIGN(Options); It feels a bit odd to use this macro and also define move and assignment operators:)
The CQ bit was checked by altimin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/1461693003/#ps220001 (title: "Even more minor fixes")
The CQ bit was unchecked by altimin@chromium.org
PTAL https://codereview.chromium.org/1461693003/diff/200001/headless/public/headle... File headless/public/headless_browser.h (right): https://codereview.chromium.org/1461693003/diff/200001/headless/public/headle... headless/public/headless_browser.h:68: Options(Options&&); On 2015/12/01 20:07:30, alexclarke1 wrote: > Do we actually need an explicit move and assignment constructor? Particularly > for a class like this which will probably be created once? Thanks for the remark. The reason behind explicit constructors are that Options used to have unique_ptr inside, thus making impossible to copy it. Chromium style guide requires and explicit destructor, and classes with explicit destructors do not receive implicit move constructors as paragraph 12.8.9 of the Standard). Since we don't have unique_ptrs inside anymore, we can avoid copying here altogether. https://codereview.chromium.org/1461693003/diff/200001/headless/public/headle... headless/public/headless_browser.h:91: DISALLOW_COPY_AND_ASSIGN(Options); On 2015/12/01 20:07:30, alexclarke1 wrote: > It feels a bit odd to use this macro and also define move and assignment > operators:) Done.
https://codereview.chromium.org/1461693003/diff/200001/headless/public/headle... File headless/public/headless_browser.h (right): https://codereview.chromium.org/1461693003/diff/200001/headless/public/headle... headless/public/headless_browser.h:109: DISALLOW_COPY_AND_ASSIGN(Builder); Is there a reason we couldn't keep this?
https://codereview.chromium.org/1461693003/diff/200001/headless/public/headle... File headless/public/headless_browser.h (right): https://codereview.chromium.org/1461693003/diff/200001/headless/public/headle... headless/public/headless_browser.h:109: DISALLOW_COPY_AND_ASSIGN(Builder); On 2015/12/02 13:37:14, Sami wrote: > Is there a reason we couldn't keep this? My bad, I wanted to remove this from Options.
lgtm
lgtm
lgtm, thanks for the improvements. Details will change but this seems like a good starting point.
The CQ bit was checked by altimin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1461693003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1461693003/240001
Message was sent while issue was closed.
Description was changed from ========== [headless] Initial skeleton of headless/public/ Create outline of future Headless API. BUG=546953 ========== to ========== [headless] Initial skeleton of headless/public/ Create outline of future Headless API. BUG=546953 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== [headless] Initial skeleton of headless/public/ Create outline of future Headless API. BUG=546953 ========== to ========== [headless] Initial skeleton of headless/public/ Create outline of future Headless API. BUG=546953 Committed: https://crrev.com/168fe2a9a59963bfd6a8d5b4762e316b3fbaf076 Cr-Commit-Position: refs/heads/master@{#362712} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/168fe2a9a59963bfd6a8d5b4762e316b3fbaf076 Cr-Commit-Position: refs/heads/master@{#362712}
Message was sent while issue was closed.
nick@chromium.org changed reviewers: + nick@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1461693003/diff/240001/headless/public/web_co... File headless/public/web_contents.h (right): https://codereview.chromium.org/1461693003/diff/240001/headless/public/web_co... headless/public/web_contents.h:25: class HEADLESS_EXPORT WebContents { I worry that having two "web_contents.h" headers in the codebase will confuse people encountering snippets of the code (I've opened the wrong one by mistake a couple of times now, though that's mostly a side effect of how I use my editor). Any reason not to call this HeadlessContents?
Message was sent while issue was closed.
On 2016/01/20 22:23:20, ncarter wrote: > https://codereview.chromium.org/1461693003/diff/240001/headless/public/web_co... > File headless/public/web_contents.h (right): > > https://codereview.chromium.org/1461693003/diff/240001/headless/public/web_co... > headless/public/web_contents.h:25: class HEADLESS_EXPORT WebContents { > I worry that having two "web_contents.h" headers in the codebase will confuse > people encountering snippets of the code (I've opened the wrong one by mistake a > couple of times now, though that's mostly a side effect of how I use my editor). > > Any reason not to call this HeadlessContents? Yeah, it's too bad namespaces don't really solve this problem. I think we could rename this to HeadlessWebContents or something like that. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
