Support multiple sign-in for extension updates.

Support multiple sign-in for extension updates.

This changes the ExtensionDownloader to attempt multiple
sequentual queries for CRX downloads, iterating over incremental
values for the authuser query parameter (up to a max of 10).

BUG=370964
R=asargent@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269340

[Ken Rockot(use gerrit already), 2014-05-08 01:03:50]

Antony, could you please take a look at this?

[asargent_no_longer_on_chrome, 2014-05-08 04:27:10]

https://codereview.chromium.org/279453002/diff/1/chrome/browser/extensions/up...
File chrome/browser/extensions/updater/extension_downloader.cc (right):

https://codereview.chromium.org/279453002/diff/1/chrome/browser/extensions/up...
chrome/browser/extensions/updater/extension_downloader.cc:117: (response_code ==
401 || response_code == 403);
optional: if you add parentheses around the entire expression, the wrapping will
look nicer and makes the code easier to read IMHO:

 return (status.status() == net::URLRequestStatus::SUCCESS &&
         (response_code == 401 || response_code == 403));

https://codereview.chromium.org/279453002/diff/1/chrome/browser/extensions/up...
chrome/browser/extensions/updater/extension_downloader.cc:129: response_code ==
403 && included_cookies;
same here

https://codereview.chromium.org/279453002/diff/1/chrome/browser/extensions/up...
chrome/browser/extensions/updater/extension_downloader.cc:161: *url =
url->Resolve(new_query);
Hmm, I assume you've actually run this and it works? I just read the comments on
the GURL::Resolve declaration and it sounded to me like this might not be the
kind of purpose it was meant for; rather it sounds like it is more intended for
coming up with new urls for relative paths based on the url of the current page
(e.g. "http://example.com/foo/bar.html has a <img src="baz.jpg"> tag where we
want to come up with "http://example.com/foo/baz.jpg" for resource loading
purposes).

Did you consider using url::ReplaceComponents with a url::Replacements value?
(defined in url/url_util.h and url/url_canon.h respectively) I think that's the
more typical way I've seen url modification done in areas of the code I've
worked in. 

(BTW, I've never been a big fan of our GURL library because it makes many simple
things so hard to do. I guess some of it is architected the way it is for
extreme performance in doing manipulations on really big urls and/or lots of
operations in latency sensitive areas; it may have even been opened sourced
based partly on google's closed source library used for a web crawler or
something, I'm not sure).

https://codereview.chromium.org/279453002/diff/1/chrome/browser/extensions/up...
File chrome/browser/extensions/updater/extension_updater_unittest.cc (right):

https://codereview.chromium.org/279453002/diff/1/chrome/browser/extensions/up...
chrome/browser/extensions/updater/extension_updater_unittest.cc:1650:
TEST_F(ExtensionUpdaterTest,
TestSingleProtectedExtensionDownloadingWithNonDefaultAuthUser1) {
nit: Can probably omit the prefix "Test" from the test case names here to make
them shorter / less redundant (we already said TEST_F and ExtensionUpdaterTest),

Also, will our 'gitl cl format' overlord allow you to format this as:

TEST_F(ExtensionUpdaterTest, 
       SingleProtectedExtensionDownloadingWithNonDefaultAuthUser1) {

to stay under 80 cols?

(similarly below)

[Ken Rockot(use gerrit already), 2014-05-08 05:23:09]

Thanks. I ran clang_format over this, so there's a little extra noise around the
changes that address comments.

https://codereview.chromium.org/279453002/diff/1/chrome/browser/extensions/up...
File chrome/browser/extensions/updater/extension_downloader.cc (right):

https://codereview.chromium.org/279453002/diff/1/chrome/browser/extensions/up...
chrome/browser/extensions/updater/extension_downloader.cc:117: (response_code ==
401 || response_code == 403);
On 2014/05/08 04:27:11, Antony Sargent wrote:
> optional: if you add parentheses around the entire expression, the wrapping
will
> look nicer and makes the code easier to read IMHO:
> 
>  return (status.status() == net::URLRequestStatus::SUCCESS &&
>          (response_code == 401 || response_code == 403));

Done.

https://codereview.chromium.org/279453002/diff/1/chrome/browser/extensions/up...
chrome/browser/extensions/updater/extension_downloader.cc:129: response_code ==
403 && included_cookies;
On 2014/05/08 04:27:11, Antony Sargent wrote:
> same here

Done.

https://codereview.chromium.org/279453002/diff/1/chrome/browser/extensions/up...
chrome/browser/extensions/updater/extension_downloader.cc:161: *url =
url->Resolve(new_query);
On 2014/05/08 04:27:11, Antony Sargent wrote:
> Hmm, I assume you've actually run this and it works? I just read the comments
on
> the GURL::Resolve declaration and it sounded to me like this might not be the
> kind of purpose it was meant for; rather it sounds like it is more intended
for
> coming up with new urls for relative paths based on the url of the current
page
> (e.g. "http://example.com/foo/bar.html has a <img src="baz.jpg"> tag where we
> want to come up with "http://example.com/foo/baz.jpg" for resource loading
> purposes).
> 
> Did you consider using url::ReplaceComponents with a url::Replacements value?
> (defined in url/url_util.h and url/url_canon.h respectively) I think that's
the
> more typical way I've seen url modification done in areas of the code I've
> worked in. 
> 
> (BTW, I've never been a big fan of our GURL library because it makes many
simple
> things so hard to do. I guess some of it is architected the way it is for
> extreme performance in doing manipulations on really big urls and/or lots of
> operations in latency sensitive areas; it may have even been opened sourced
> based partly on google's closed source library used for a web crawler or
> something, I'm not sure). 

Yeah, using Resolve is OK in this case, but I actually like ReplaceComponents
(at least, I like it more than Resolve), so I've switched to that. I didn't
realize until trying to write this CL just how useless GURL is.

https://codereview.chromium.org/279453002/diff/1/chrome/browser/extensions/up...
File chrome/browser/extensions/updater/extension_updater_unittest.cc (right):

https://codereview.chromium.org/279453002/diff/1/chrome/browser/extensions/up...
chrome/browser/extensions/updater/extension_updater_unittest.cc:1650:
TEST_F(ExtensionUpdaterTest,
TestSingleProtectedExtensionDownloadingWithNonDefaultAuthUser1) {
On 2014/05/08 04:27:11, Antony Sargent wrote:
> nit: Can probably omit the prefix "Test" from the test case names here to make
> them shorter / less redundant (we already said TEST_F and
ExtensionUpdaterTest),
> 
> Also, will our 'gitl cl format' overlord allow you to format this as:
> 
> TEST_F(ExtensionUpdaterTest, 
>        SingleProtectedExtensionDownloadingWithNonDefaultAuthUser1) {
> 
> to stay under 80 cols?
> 
> (similarly below)
> 

Done.

[asargent_no_longer_on_chrome, 2014-05-08 15:55:46]

lgtm

[Ken Rockot(use gerrit already), 2014-05-08 16:18:39]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-05-08 16:22:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rockot@chromium.org/279453002/20001

[commit-bot: I haz the power, 2014-05-09 04:54:11]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-09 16:53:27]

Change committed as 269340

[mmenke, 2014-05-09 19:57:04]

On 2014/05/09 16:53:27, I haz the power (commit-bot) wrote:
> Change committed as 269340

I'm going to revert, as these tests are failing on Linux
(http://build.chromium.org/p/chromium.linux/buildstatus?builder=Linux%20Clang%...)