DevTools: Introduce Page.getManifest remote debugging protocol method.

DevTools: Introduce Page.getManifest remote debugging protocol method.

BUG=607779
Committed: https://crrev.com/d9e2f6578d27b98d379ea9abb5bbf4d1b630b727
Cr-Commit-Position: refs/heads/master@{#390754}

[pfeldman, 2016-04-29 02:16:50]

pfeldman@chromium.org changed reviewers:
+ dgozman@chromium.org

[dgozman, 2016-04-29 02:28:26]

lgtm

https://codereview.chromium.org/1932623003/diff/20001/content/renderer/devtoo...
File content/renderer/devtools/devtools_agent.cc (right):

https://codereview.chromium.org/1932623003/diff/20001/content/renderer/devtoo...
content/renderer/devtools/devtools_agent.cc:296: for (const auto& error
:debug_info.errors) {
style: space after colon

https://codereview.chromium.org/1932623003/diff/20001/content/renderer/manife...
File content/renderer/manifest/manifest_manager.cc (left):

https://codereview.chromium.org/1932623003/diff/20001/content/renderer/manife...
content/renderer/manifest/manifest_manager.cc:192: Manifest manifest =
manifest_;
Maybe |manifest_| could change during callback, and so it was saved to a var?

https://codereview.chromium.org/1932623003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/protocol.json (right):

https://codereview.chromium.org/1932623003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/protocol.json:458: "handlers": ["none"]
interceptor? manual?

[pfeldman, 2016-04-29 02:36:18]

https://codereview.chromium.org/1932623003/diff/20001/content/renderer/manife...
File content/renderer/manifest/manifest_manager.cc (left):

https://codereview.chromium.org/1932623003/diff/20001/content/renderer/manife...
content/renderer/manifest/manifest_manager.cc:192: Manifest manifest =
manifest_;
On 2016/04/29 02:28:26, dgozman wrote:
> Maybe |manifest_| could change during callback, and so it was saved to a var?

Originally, I did not see how it was possible, but now I can see how existing
code is racy. If navigation commits between request and response,
ResolveCallbacks would overwrite manifest_dirty_ and we would end up with the
old manifest, etc.

[pfeldman, 2016-04-29 02:44:19]

pfeldman@chromium.org changed reviewers:
+ mlamouri@chromium.org

[pfeldman, 2016-04-29 02:44:20]

mlamouri => manifest

[mlamouri (slow - plz ping), 2016-04-29 11:14:12]

lgtm

https://codereview.chromium.org/1932623003/diff/40001/content/renderer/manife...
File content/renderer/manifest/manifest_debug_info.cc (right):

https://codereview.chromium.org/1932623003/diff/40001/content/renderer/manife...
content/renderer/manifest/manifest_debug_info.cc:9:
ManifestDebugInfo::ManifestDebugInfo() {
nit: = default;

https://codereview.chromium.org/1932623003/diff/40001/content/renderer/manife...
content/renderer/manifest/manifest_debug_info.cc:12:
ManifestDebugInfo::~ManifestDebugInfo() {
nit = default;

https://codereview.chromium.org/1932623003/diff/40001/content/renderer/manife...
File content/renderer/manifest/manifest_debug_info.h (right):

https://codereview.chromium.org/1932623003/diff/40001/content/renderer/manife...
content/renderer/manifest/manifest_debug_info.h:13: struct ManifestDebugInfo {
style: can you add a comment explaining the purpose of the class.

https://codereview.chromium.org/1932623003/diff/40001/content/renderer/manife...
content/renderer/manifest/manifest_debug_info.h:15: Error(const std::string&
message, int line, int column)
qq: could you take a && and do the same for Manifest::AddError(). Seems like we
could avoid one string copy. No need to do that in this CL though. Just a
thought.

[pfeldman, 2016-04-29 18:47:53]

https://codereview.chromium.org/1932623003/diff/40001/content/renderer/manife...
File content/renderer/manifest/manifest_debug_info.cc (right):

https://codereview.chromium.org/1932623003/diff/40001/content/renderer/manife...
content/renderer/manifest/manifest_debug_info.cc:9:
ManifestDebugInfo::ManifestDebugInfo() {
On 2016/04/29 11:14:12, Mounir Lamouri wrote:
> nit: = default;

Done.

https://codereview.chromium.org/1932623003/diff/40001/content/renderer/manife...
content/renderer/manifest/manifest_debug_info.cc:12:
ManifestDebugInfo::~ManifestDebugInfo() {
On 2016/04/29 11:14:12, Mounir Lamouri wrote:
> nit = default;

Done.

https://codereview.chromium.org/1932623003/diff/40001/content/renderer/manife...
File content/renderer/manifest/manifest_debug_info.h (right):

https://codereview.chromium.org/1932623003/diff/40001/content/renderer/manife...
content/renderer/manifest/manifest_debug_info.h:13: struct ManifestDebugInfo {
On 2016/04/29 11:14:12, Mounir Lamouri wrote:
> style: can you add a comment explaining the purpose of the class.

Done.

https://codereview.chromium.org/1932623003/diff/40001/content/renderer/manife...
content/renderer/manifest/manifest_debug_info.h:15: Error(const std::string&
message, int line, int column)
On 2016/04/29 11:14:12, Mounir Lamouri wrote:
> qq: could you take a && and do the same for Manifest::AddError(). Seems like
we
> could avoid one string copy. No need to do that in this CL though. Just a
> thought.

Ack.

[pfeldman, 2016-04-29 18:48:02]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-04-29 18:48:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1932623003/#ps80001
(title: "errors/warnings split")

[commit-bot: I haz the power, 2016-04-29 18:48:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1932623003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1932623003/80001

[commit-bot: I haz the power, 2016-04-29 19:14:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-29 19:14:26]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pfeldman, 2016-04-29 19:41:53]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2016-04-29 19:41:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
dgozman@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/1932623003/#ps100001
(title: "browser test updated")

[commit-bot: I haz the power, 2016-04-29 19:42:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1932623003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1932623003/100001

[commit-bot: I haz the power, 2016-04-29 20:58:21]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-30 17:28:47]

Patchset 6 (id:??) landed as
https://crrev.com/d9e2f6578d27b98d379ea9abb5bbf4d1b630b727
Cr-Commit-Position: refs/heads/master@{#390754}

[commit-bot: I haz the power, 2016-04-30 18:02:30]

Description was changed from

==========
DevTools: Introduce Page.getManifest remote debugging protocol method.

BUG=607779
==========

to

==========
DevTools: Introduce Page.getManifest remote debugging protocol method.

BUG=607779

Committed: https://crrev.com/d9e2f6578d27b98d379ea9abb5bbf4d1b630b727
Cr-Commit-Position: refs/heads/master@{#390754}
==========