DevTools: Fix getCookies to report for all resources

DevTools: Fix getCookies to report for all resources

* Changes getCookies protocol method to accept a set of urls
* Changes getCookiesAsync to request specific urls from a specific target
* Changes CookieItemsView to fetch cookies for all subresources of the frame

BUG=673934
Review-Url: https://codereview.chromium.org/2623063003
Cr-Commit-Position: refs/heads/master@{#445464}
Committed: https://chromium.googlesource.com/chromium/src/+/36ab298cb853d6bf6366065dee9821dfd42c69c3

[phulce, 2017-01-12 23:58:54]

Description was changed from

==========
DevTools: Fix getCookies to report for all resources

* Changes getCookies protocol method to accept a set of urls
* Changes getCookiesAsync to request specific urls from a specific target
* Changes CookieItemsView to fetch cookies for all subresources of the frame

BUG=673934
==========

to

==========
DevTools: Fix getCookies to report for all resources

* Changes getCookies protocol method to accept a set of urls
* Changes getCookiesAsync to request specific urls from a specific target
* Changes CookieItemsView to fetch cookies for all subresources of the frame

BUG=673934
==========

[phulce, 2017-01-12 23:58:54]

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

[phulce, 2017-01-13 00:00:58]

Currently blocked on 680054 but a fix is pending in 2627073003.

Not sure what the process is for breaking changes to protocol or if there are
other uses outside devtools/front_end.

[pfeldman, 2017-01-13 02:17:34]

https://codereview.chromium.org/2623063003/diff/20001/content/browser/devtool...
File content/browser/devtools/protocol/network_handler.cc (right):

https://codereview.chromium.org/2623063003/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:221:
BrowserThread::PostTask(
You should not post as many tasks as there are urls, pass array into a single
task instead.

Also, you might need to maintain existing behavior of url gathering in case no
urls are given in the protocol command. So you can gather first and post the
task after.

https://codereview.chromium.org/2623063003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2623063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1235:
"parameters": [
List parameters before "returns". You should also mark urls as optional since
existing clients are not going to pass urls parameter. That will change the
handler method signature to accept nullable parameter. In case of null, you
should fall back to default behavior.

https://codereview.chromium.org/2623063003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js (right):

https://codereview.chromium.org/2623063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:1317: const
nonDataUrls = requests.map(r => r.url()).filter(url => url &&
!/^data:/.test(url));
Use ParsedURL to figure out url type.

https://codereview.chromium.org/2623063003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js
(right):

https://codereview.chromium.org/2623063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js:104:
function populateResourceURLs(resource) {
You should annotate all function parameters for compilation.

https://codereview.chromium.org/2623063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js:119:
this._totalSize = allCookies.reduce((size, cookie) => size + cookie.size(), 0);
Not sure why total size would matter :)

https://codereview.chromium.org/2623063003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js
(right):

https://codereview.chromium.org/2623063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:393:
addCookieDocument(frame) {
Annotation missing - you touched it...

[phulce, 2017-01-18 00:52:34]

https://codereview.chromium.org/2623063003/diff/20001/content/browser/devtool...
File content/browser/devtools/protocol/network_handler.cc (right):

https://codereview.chromium.org/2623063003/diff/20001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:221:
BrowserThread::PostTask(
On 2017/01/13 02:17:34, pfeldman wrote:
> You should not post as many tasks as there are urls, pass array into a single
> task instead.
> 
> Also, you might need to maintain existing behavior of url gathering in case no
> urls are given in the protocol command. So you can gather first and post the
> task after.

Done.

https://codereview.chromium.org/2623063003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol.json (right):

https://codereview.chromium.org/2623063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol.json:1235:
"parameters": [
On 2017/01/13 02:17:34, pfeldman wrote:
> List parameters before "returns". You should also mark urls as optional since
> existing clients are not going to pass urls parameter. That will change the
> handler method signature to accept nullable parameter. In case of null, you
> should fall back to default behavior.

Done.

https://codereview.chromium.org/2623063003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js (right):

https://codereview.chromium.org/2623063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/audits/AuditRules.js:1317: const
nonDataUrls = requests.map(r => r.url()).filter(url => url &&
!/^data:/.test(url));
On 2017/01/13 02:17:34, pfeldman wrote:
> Use ParsedURL to figure out url type.

Done.

https://codereview.chromium.org/2623063003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js
(right):

https://codereview.chromium.org/2623063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js:104:
function populateResourceURLs(resource) {
On 2017/01/13 02:17:34, pfeldman wrote:
> You should annotate all function parameters for compilation.

Done.

https://codereview.chromium.org/2623063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js:119:
this._totalSize = allCookies.reduce((size, cookie) => size + cookie.size(), 0);
On 2017/01/13 02:17:34, pfeldman wrote:
> Not sure why total size would matter :)

it's pre-existing, used for display

https://codereview.chromium.org/2623063003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js
(right):

https://codereview.chromium.org/2623063003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/resources/ResourcesPanel.js:393:
addCookieDocument(frame) {
On 2017/01/13 02:17:34, pfeldman wrote:
> Annotation missing - you touched it...

Done.

[pfeldman, 2017-01-18 01:41:11]

https://codereview.chromium.org/2623063003/diff/40001/content/browser/devtool...
File content/browser/devtools/protocol/network_handler.cc (right):

https://codereview.chromium.org/2623063003/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:37: class CookieReceiver {
nit: inherit from base::NonThreadSafe and DCHECK(CalledOnValidThread()) below.

nit: also, making it ref-counted-thread-safe would allow graceful threads
termination.

https://codereview.chromium.org/2623063003/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:43: request_count_ +=
count;
nit: use base::BarrierClosure instead - your callback would receive it, do the
work and call barrier's closure.

https://codereview.chromium.org/2623063003/diff/40001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:245:
&GetCookiesCommand::SendResponse, base::Unretained(this));
This is dangerous - when your callback triggers, this object can no longer be
there. You need to use WeakPtr factory here.

[phulce, 2017-01-19 17:13:24]

On 2017/01/18 01:41:11, pfeldman wrote:
>
https://codereview.chromium.org/2623063003/diff/40001/content/browser/devtool...
> File content/browser/devtools/protocol/network_handler.cc (right):
> 
>
https://codereview.chromium.org/2623063003/diff/40001/content/browser/devtool...
> content/browser/devtools/protocol/network_handler.cc:37: class CookieReceiver
{
> nit: inherit from base::NonThreadSafe and DCHECK(CalledOnValidThread()) below.
> 
> nit: also, making it ref-counted-thread-safe would allow graceful threads
> termination.
> 
>
https://codereview.chromium.org/2623063003/diff/40001/content/browser/devtool...
> content/browser/devtools/protocol/network_handler.cc:43: request_count_ +=
> count;
> nit: use base::BarrierClosure instead - your callback would receive it, do the
> work and call barrier's closure.
> 
>
https://codereview.chromium.org/2623063003/diff/40001/content/browser/devtool...
> content/browser/devtools/protocol/network_handler.cc:245:
> &GetCookiesCommand::SendResponse, base::Unretained(this));
> This is dangerous - when your callback triggers, this object can no longer be
> there. You need to use WeakPtr factory here.

Addressed all in-person feedback

[pfeldman, 2017-01-19 18:58:35]

lgtm % comments. you don't need to fix the nits, those are mostly code style
fyi.

https://codereview.chromium.org/2623063003/diff/60001/content/browser/devtool...
File content/browser/devtools/protocol/network_handler.cc (right):

https://codereview.chromium.org/2623063003/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:71: if (callback_count_ ==
0) {
nit: some would say !callback_count_.

https://codereview.chromium.org/2623063003/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:108: if (callback_count_ ==
0) {
nit: some would drop {} and inline --callback_count_

https://codereview.chromium.org/2623063003/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:115: for (const auto& pair
: cookies_) {
nit: ditto

https://codereview.chromium.org/2623063003/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:160: if (callback_ !=
nullptr) {
if (callback_)

https://codereview.chromium.org/2623063003/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:162: } else if
(all_callback_ != nullptr) {
} else {
  DCHECK(all_callback_);
  ...
}

https://codereview.chromium.org/2623063003/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:169: int callback_count_;
in the new c++ you can say int callback_count_ = 0; and drop it from super
initializers.

https://codereview.chromium.org/2623063003/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:170:
base::hash_map<std::string, net::CanonicalCookie> cookies_;
use std::unordered_map (hash_map is just an alias)

https://codereview.chromium.org/2623063003/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:292: static
std::vector<GURL> ComputeCookieURLs(
nit: since you have anonymous namespace declared few lines above, you can move
this method there and drop 'static' modifier. it'll result in the same thing.

https://codereview.chromium.org/2623063003/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:311: if
(node->current_frame_host()->IsCrossProcessSubframe())
nit: you don't need this check, we can report cross process for now.

https://codereview.chromium.org/2623063003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/inspector/browser_protocol-1.2.json (right):

https://codereview.chromium.org/2623063003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/inspector/browser_protocol-1.2.json:1243:
"parameters": [
don't need to modify this

[phulce, 2017-01-19 21:18:58]

https://codereview.chromium.org/2623063003/diff/60001/content/browser/devtool...
File content/browser/devtools/protocol/network_handler.cc (right):

https://codereview.chromium.org/2623063003/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:71: if (callback_count_ ==
0) {
On 2017/01/19 18:58:35, pfeldman wrote:
> nit: some would say !callback_count_.

Acknowledged.

https://codereview.chromium.org/2623063003/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:108: if (callback_count_ ==
0) {
On 2017/01/19 18:58:35, pfeldman wrote:
> nit: some would drop {} and inline --callback_count_

Done.

https://codereview.chromium.org/2623063003/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:115: for (const auto& pair
: cookies_) {
On 2017/01/19 18:58:35, pfeldman wrote:
> nit: ditto

Done.

https://codereview.chromium.org/2623063003/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:160: if (callback_ !=
nullptr) {
On 2017/01/19 18:58:35, pfeldman wrote:
> if (callback_)

Done.

https://codereview.chromium.org/2623063003/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:162: } else if
(all_callback_ != nullptr) {
On 2017/01/19 18:58:35, pfeldman wrote:
> } else {
>   DCHECK(all_callback_);
>   ...
> }

Done.

https://codereview.chromium.org/2623063003/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:169: int callback_count_;
On 2017/01/19 18:58:35, pfeldman wrote:
> in the new c++ you can say int callback_count_ = 0; and drop it from super
> initializers.

Done.

https://codereview.chromium.org/2623063003/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:170:
base::hash_map<std::string, net::CanonicalCookie> cookies_;
On 2017/01/19 18:58:35, pfeldman wrote:
> use std::unordered_map (hash_map is just an alias)

Done.

https://codereview.chromium.org/2623063003/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:292: static
std::vector<GURL> ComputeCookieURLs(
On 2017/01/19 18:58:35, pfeldman wrote:
> nit: since you have anonymous namespace declared few lines above, you can move
> this method there and drop 'static' modifier. it'll result in the same thing.

Done.

https://codereview.chromium.org/2623063003/diff/60001/content/browser/devtool...
content/browser/devtools/protocol/network_handler.cc:311: if
(node->current_frame_host()->IsCrossProcessSubframe())
On 2017/01/19 18:58:35, pfeldman wrote:
> nit: you don't need this check, we can report cross process for now.

Done.

[pfeldman, 2017-01-20 23:13:41]

still lgtm

[phulce, 2017-01-21 00:50:20]

The CQ bit was checked by phulce@chromium.org

[commit-bot: I haz the power, 2017-01-21 00:50:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-21 02:22:26]

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

[commit-bot: I haz the power, 2017-01-21 02:22:28]

Failed to apply patch for
third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js:
While running git apply --index -p1;
  error: patch failed:
third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js:123
  error:
third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js: patch
does not apply

Patch:      
third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js
Index: third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js
diff --git
a/third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js
b/third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js
index
74b1d1141472261a12a81f48b0941f407d3b4825..47da4f0a3c76ff1bc25964c56d61aaf2ae0a51f0
100644
--- a/third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js
+++ b/third_party/WebKit/Source/devtools/front_end/resources/CookieItemsView.js
@@ -31,7 +31,7 @@
  * @unrestricted
  */
 Resources.CookieItemsView = class extends UI.SimpleView {
-  constructor(treeElement, cookieDomain) {
+  constructor(treeElement, target, cookieDomain) {
     super(Common.UIString('Cookies'));
 
     this.element.classList.add('storage-view');
@@ -53,6 +53,7 @@ Resources.CookieItemsView = class extends UI.SimpleView {
     this._filterSeparator = new UI.ToolbarSeparator();
     this._filterButton = this._filterBar.filterButton();
 
+    this._target = target;
     this._treeElement = treeElement;
     this._cookieDomain = cookieDomain;
 
@@ -98,14 +99,27 @@ Resources.CookieItemsView = class extends UI.SimpleView {
   }
 
   _update() {
-    SDK.Cookies.getCookiesAsync(this._updateWithCookies.bind(this));
+    var resourceURLs = [];
+    var cookieDomain = this._cookieDomain;
+    /**
+     * @param {!SDK.Resource} resource
+     */
+    function populateResourceURLs(resource) {
+      var url = resource.documentURL.asParsedURL();
+      if (url && url.securityOrigin() === cookieDomain)
+        resourceURLs.push(resource.url);
+    }
+
+   
SDK.ResourceTreeModel.fromTarget(this._target).forAllResources(populateResourceURLs);
+    SDK.Cookies.getCookiesAsync(this._target, resourceURLs,
this._updateWithCookies.bind(this));
   }
 
   /**
    * @param {!Array.<!SDK.Cookie>} allCookies
    */
   _updateWithCookies(allCookies) {
-    this._cookies = this._filterCookiesForDomain(allCookies);
+    this._cookies = allCookies;
+    this._totalSize = allCookies.reduce((size, cookie) => size + cookie.size(),
0);
 
     if (!this._cookies.length) {
       // Nothing to show.
@@ -123,7 +137,7 @@ Resources.CookieItemsView = class extends UI.SimpleView {
           new CookieTable.CookiesTable(false, this._update.bind(this),
this._enableDeleteButton.bind(this));
     }
 
-    var shownCookies = this._filterCookiesForFilters(this._cookies);
+    var shownCookies = this._filterCookies(this._cookies);
     this._cookiesTable.setCookies(shownCookies);
     this._emptyWidget.detach();
     this._cookiesTable.show(this.element);
@@ -138,7 +152,7 @@ Resources.CookieItemsView = class extends UI.SimpleView {
   /**
    * @param {!Array.<!SDK.Cookie>} cookies
    */
-  _filterCookiesForFilters(cookies) {
+  _filterCookies(cookies) {
     if (!this._filterRegex)
       return cookies;
 
@@ -148,41 +162,6 @@ Resources.CookieItemsView = class extends UI.SimpleView {
     });
   }
 
-  /**
-   * @param {!Array.<!SDK.Cookie>} allCookies
-   */
-  _filterCookiesForDomain(allCookies) {
-    var cookies = [];
-    var resourceURLsForDocumentURL = [];
-    this._totalSize = 0;
-
-    /**
-     * @this {Resources.CookieItemsView}
-     */
-    function populateResourcesForDocuments(resource) {
-      var url = resource.documentURL.asParsedURL();
-      if (url && url.securityOrigin() === this._cookieDomain)
-        resourceURLsForDocumentURL.push(resource.url);
-    }
-    Bindings.forAllResources(populateResourcesForDocuments.bind(this));
-
-    for (var i = 0; i < allCookies.length; ++i) {
-      var pushed = false;
-      var size = allCookies[i].size();
-      for (var j = 0; j < resourceURLsForDocumentURL.length; ++j) {
-        var resourceURL = resourceURLsForDocumentURL[j];
-        if (SDK.Cookies.cookieMatchesResourceURL(allCookies[i], resourceURL)) {
-          this._totalSize += size;
-          if (!pushed) {
-            pushed = true;
-            cookies.push(allCookies[i]);
-          }
-        }
-      }
-    }
-    return cookies;
-  }
-
   clear() {
     this._cookiesTable.clear();
     this._update();

[phulce, 2017-01-23 18:26:42]

The CQ bit was checked by phulce@chromium.org

[phulce, 2017-01-23 18:26:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2623063003/#ps100001
(title: "rebase master")

[commit-bot: I haz the power, 2017-01-23 18:27:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 20:23:40]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485196002239210,
"parent_rev": "06b77196ee13b55a2b1831ae1f2bcd1b5b8b917f", "commit_rev":
"36ab298cb853d6bf6366065dee9821dfd42c69c3"}

[commit-bot: I haz the power, 2017-01-23 20:24:18]

Description was changed from

==========
DevTools: Fix getCookies to report for all resources

* Changes getCookies protocol method to accept a set of urls
* Changes getCookiesAsync to request specific urls from a specific target
* Changes CookieItemsView to fetch cookies for all subresources of the frame

BUG=673934
==========

to

==========
DevTools: Fix getCookies to report for all resources

* Changes getCookies protocol method to accept a set of urls
* Changes getCookiesAsync to request specific urls from a specific target
* Changes CookieItemsView to fetch cookies for all subresources of the frame

BUG=673934

Review-Url: https://codereview.chromium.org/2623063003
Cr-Commit-Position: refs/heads/master@{#445464}
Committed:
https://chromium.googlesource.com/chromium/src/+/36ab298cb853d6bf6366065dee98...
==========

[commit-bot: I haz the power, 2017-01-23 20:24:20]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/36ab298cb853d6bf6366065dee98...