|
|
Descriptionheadless: Add a small C++ example application
Headless Shell has grown to be a little too complex to serve as an
introduction to the headless C++ API. This patch adds a simpler example
which is hopefully easier to follow. The application navigates
to a URL given on the command line, waits for it to load and prints the
DOM.
BUG=546953
R=altimin@chromium.org
Review-Url: https://codereview.chromium.org/2677033003 .
Cr-Commit-Position: refs/heads/master@{#450025}
Committed: https://chromium.googlesource.com/chromium/src/+/354db138caff746a371c025aee80b830d23ee7d0
Patch Set 1 #Patch Set 2 : outerHTML instead of innerHTML #
Total comments: 12
Patch Set 3 : Review comments #Patch Set 4 : Rebased. Don't require command line to be initialized. #Patch Set 5 : Don't touch command line (already fixed elsewhere) #Patch Set 6 : Rebased #
Messages
Total messages: 34 (24 generated)
The CQ bit was checked by skyostil@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by skyostil@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== headless: Add a small C++ example application Headless Shell has grown to be a little too complex to serve as an introduction to the headless C++ API. This patch adds a simpler example which is hopefully easier to follow. The application navigates to a URL given on the command line, waits for it to load and prints the DOM. BUG=546953 ========== to ========== headless: Add a small C++ example application Headless Shell has grown to be a little too complex to serve as an introduction to the headless C++ API. This patch adds a simpler example which is hopefully easier to follow. The application navigates to a URL given on the command line, waits for it to load and prints the DOM. BUG=546953 ==========
skyostil@chromium.org changed reviewers: + alexclarke@chromium.org, altimin@chromium.org
https://codereview.chromium.org/2677033003/diff/20001/headless/app/headless_e... File headless/app/headless_example.cc (right): https://codereview.chromium.org/2677033003/diff/20001/headless/app/headless_e... headless/app/headless_example.cc:43: headless::HeadlessBrowser* browser_; UNOWNED? https://codereview.chromium.org/2677033003/diff/20001/headless/app/headless_e... headless/app/headless_example.cc:45: headless::HeadlessWebContents* web_contents_; UNOWNED? https://codereview.chromium.org/2677033003/diff/20001/headless/app/headless_e... headless/app/headless_example.cc:53: std::unique_ptr<HeadlessExample> g_example; I'm slightly terrified about the idea that HeadlessExample's destructor may be called during cleanup of global variables (it will break horribly). I recommend using raw pointer here (or base::Singleton). https://codereview.chromium.org/2677033003/diff/20001/headless/app/headless_e... headless/app/headless_example.cc:68: web_contents_->GetDevToolsTarget()->DetachClient(devtools_client_.get()); We should also add removing observer from Page domain here. Random thought: we have a number of constructions like if (page_observer) devtools_client_->GetPage()->RemoveObserver(this). We should consider adding an RAII-style handle for observers: page_observer_ = devtools_client_->GetPage()->AddObserver(this); page_observer_.reset(); https://codereview.chromium.org/2677033003/diff/20001/headless/app/headless_e... headless/app/headless_example.cc:98: std::string dom; Check that we got a valid result here?
https://codereview.chromium.org/2677033003/diff/20001/headless/app/headless_e... File headless/app/headless_example.cc (right): https://codereview.chromium.org/2677033003/diff/20001/headless/app/headless_e... headless/app/headless_example.cc:25: class HeadlessExample : public headless::HeadlessWebContents::Observer, This should probably extend inspector::ExperimentalObserver so we can add: void OnTargetCrashed(const inspector::TargetCrashedParams& params) override { LOG(ERROR) << "Abnormal renderer termination."; ... }
Thanks! https://codereview.chromium.org/2677033003/diff/20001/headless/app/headless_e... File headless/app/headless_example.cc (right): https://codereview.chromium.org/2677033003/diff/20001/headless/app/headless_e... headless/app/headless_example.cc:25: class HeadlessExample : public headless::HeadlessWebContents::Observer, On 2017/02/10 14:46:47, alex clarke wrote: > This should probably extend inspector::ExperimentalObserver so we can add: > > void OnTargetCrashed(const inspector::TargetCrashedParams& params) override { > LOG(ERROR) << "Abnormal renderer termination."; > ... > } Hmm, this is supposed to be a really simple example so I'd like to leave stuff like that out. Added a comment though. https://codereview.chromium.org/2677033003/diff/20001/headless/app/headless_e... headless/app/headless_example.cc:43: headless::HeadlessBrowser* browser_; On 2017/02/10 14:39:43, altimin wrote: > UNOWNED? I'm thinking in the age of unique_ptr and friends, "not owned" type comments aren't that helpful anymore since a naked pointer almost always means lifetime is managed by someone else. I'll add a comment saying *who* owns these though. https://codereview.chromium.org/2677033003/diff/20001/headless/app/headless_e... headless/app/headless_example.cc:45: headless::HeadlessWebContents* web_contents_; On 2017/02/10 14:39:43, altimin wrote: > UNOWNED? Done. https://codereview.chromium.org/2677033003/diff/20001/headless/app/headless_e... headless/app/headless_example.cc:53: std::unique_ptr<HeadlessExample> g_example; On 2017/02/10 14:39:43, altimin wrote: > I'm slightly terrified about the idea that HeadlessExample's destructor may be > called during cleanup of global variables (it will break horribly). I recommend > using raw pointer here (or base::Singleton). Good point, let's make this a raw ptr. https://codereview.chromium.org/2677033003/diff/20001/headless/app/headless_e... headless/app/headless_example.cc:68: web_contents_->GetDevToolsTarget()->DetachClient(devtools_client_.get()); On 2017/02/10 14:39:43, altimin wrote: > We should also add removing observer from Page domain here. Oops, done. > Random thought: we have a number of constructions like if (page_observer) > devtools_client_->GetPage()->RemoveObserver(this). We should consider adding an > RAII-style handle for observers: > > page_observer_ = devtools_client_->GetPage()->AddObserver(this); > > page_observer_.reset(); Not a bad idea. I guess that would be some kind of observer handle. The other option would be to make AddObserver take a weak pointer. https://codereview.chromium.org/2677033003/diff/20001/headless/app/headless_e... headless/app/headless_example.cc:98: std::string dom; On 2017/02/10 14:39:43, altimin wrote: > Check that we got a valid result here? Done.
The CQ bit was checked by skyostil@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by skyostil@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
thanks, lgtm!
The CQ bit was checked by skyostil@chromium.org
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
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 on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by skyostil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from altimin@chromium.org Link to the patchset: https://codereview.chromium.org/2677033003/#ps100001 (title: "Rebased")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== headless: Add a small C++ example application Headless Shell has grown to be a little too complex to serve as an introduction to the headless C++ API. This patch adds a simpler example which is hopefully easier to follow. The application navigates to a URL given on the command line, waits for it to load and prints the DOM. BUG=546953 ========== to ========== headless: Add a small C++ example application Headless Shell has grown to be a little too complex to serve as an introduction to the headless C++ API. This patch adds a simpler example which is hopefully easier to follow. The application navigates to a URL given on the command line, waits for it to load and prints the DOM. BUG=546953 R=altimin@chromium.org Review-Url: https://codereview.chromium.org/2677033003 . Cr-Commit-Position: refs/heads/master@{#450025} Committed: https://chromium.googlesource.com/chromium/src/+/354db138caff746a371c025aee80... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) manually as 354db138caff746a371c025aee80b830d23ee7d0. |