Get rid of net::DO_NOT_PROMPT_FOR_LOGIN

Get rid of net::DO_NOT_PROMPT_FOR_LOGIN

Used only in content layer

BUG=426442
Committed: https://crrev.com/1b32ffb4800e222e3a9c92f78081246b02e83ec5
Cr-Commit-Position: refs/heads/master@{#311665}

[baranovich, 2014-12-09 23:18:35]

baranovich@yandex-team.ru changed reviewers:
+ bengr@chromium.org, benjhayden@chromium.org, jam@chromium.org,
mmenke@chromium.org, ricea@chromium.org, thakis@chromium.org

[baranovich, 2014-12-09 23:20:18]

Adding a lot of reviewers, since there are a lot of folders affected:)

[baranovich, 2014-12-09 23:33:50]

https://codereview.chromium.org/786423002/diff/20001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/786423002/diff/20001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.cc:1238: load_flags |=
net::LOAD_DO_NOT_USE_EMBEDDED_IDENTITY;
not sure if it's the best decision. On the one hand, I want to keep the things
together, since they're together:) on the other hand, there is
BuildLoadFlagsForRequest() ti build request flags.

https://codereview.chromium.org/786423002/diff/20001/content/public/browser/d...
File content/public/browser/download_url_parameters.h (right):

https://codereview.chromium.org/786423002/diff/20001/content/public/browser/d...
content/public/browser/download_url_parameters.h:104: void
set_do_not_prompt_for_login(bool do_not_prompt) {
If this method is added then load_flags/set_load_flags are not necesssary.
Should I delete them or leave for the future?

[jam, 2014-12-10 17:53:25]

(just looked at content)

https://codereview.chromium.org/786423002/diff/20001/content/browser/loader/r...
File content/browser/loader/resource_request_info_impl.cc (right):

https://codereview.chromium.org/786423002/diff/20001/content/browser/loader/r...
content/browser/loader/resource_request_info_impl.cc:66:
blink::WebPageVisibilityStateVisible,    // visibility_state
?

https://codereview.chromium.org/786423002/diff/20001/content/public/browser/d...
File content/public/browser/download_url_parameters.h (right):

https://codereview.chromium.org/786423002/diff/20001/content/public/browser/d...
content/public/browser/download_url_parameters.h:104: void
set_do_not_prompt_for_login(bool do_not_prompt) {
On 2014/12/09 23:33:49, baranovich wrote:
> If this method is added then load_flags/set_load_flags are not necesssary.
> Should I delete them or leave for the future?

remove them

[mmenke, 2014-12-10 19:07:04]

On 2014/12/09 23:20:18, baranovich wrote:
> Adding a lot of reviewers, since there are a lot of folders affected:)

When you add a bunch of reviewers, should be clear about who should review what.
 I'm assuming I should review the whole thing.

[mmenke, 2014-12-10 19:28:30]

https://codereview.chromium.org/786423002/diff/20001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/786423002/diff/20001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.cc:1221:
IsResourceTypeFrame(request_data.resource_type);
Suggest moving the load_flags and is_sync_load stuff down here, too, to keep
related code together.  Also, the DCHECKs on what flags are set should probably
be just before the SetLoadFlags line, now that we're modifying flags here, too.

https://codereview.chromium.org/786423002/diff/20001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.cc:1224:
request_data.resource_type == RESOURCE_TYPE_FAVICON;
BUG:  You need request_data.do_not_prompt_for_login in the mix, too.  The fact
this didn't make any tests fail is a bit concerning.

I think this may be a little clearer, than just adding another || clause:

bool do_not_prompt_for_login = request_data.do_not_prompt_for_login;
if (... || ...)
  do_not_prompt_for_login = true;

https://codereview.chromium.org/786423002/diff/20001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.cc:1238: load_flags |=
net::LOAD_DO_NOT_USE_EMBEDDED_IDENTITY;
On 2014/12/09 23:33:49, baranovich wrote:
> not sure if it's the best decision. On the one hand, I want to keep the things
> together, since they're together:) on the other hand, there is
> BuildLoadFlagsForRequest() ti build request flags.

It's certainly a bit ugly, but I don't have a better suggestion.

[baranovich, 2014-12-10 22:20:37]

On 2014/12/10 19:07:04, mmenke wrote:
> On 2014/12/09 23:20:18, baranovich wrote:
> > Adding a lot of reviewers, since there are a lot of folders affected:)
> 
> When you add a bunch of reviewers, should be clear about who should review
what.
>  I'm assuming I should review the whole thing.

I was trying to be optimal in selecting reviewers. I apologize if I bothered too
many people.

For instance, you were chosen as an owner of components/captive_portal,
content/browser/loader and net, but I'll be happy if you look at the other
changes.

The rest are:

ricea@ - net/websockets

thakis@ - rlz

bengr@ - components/precache

benjhayden@ - download extension API and content

jam@ - content as a whole (content/child, content/browser/renderer_host)

[mmenke, 2014-12-10 22:23:25]

On 2014/12/10 22:20:37, baranovich wrote:
> On 2014/12/10 19:07:04, mmenke wrote:
> > On 2014/12/09 23:20:18, baranovich wrote:
> > > Adding a lot of reviewers, since there are a lot of folders affected:)
> > 
> > When you add a bunch of reviewers, should be clear about who should review
> what.
> >  I'm assuming I should review the whole thing.
> 
> I was trying to be optimal in selecting reviewers. I apologize if I bothered
too
> many people.

Nothing wrong with having a lot of reviewers, just can be difficult for them to
figure out what parts they should be reviewing.  I could easily have missed the
single file in net/ or in captive_portal/, for instance.

> 
> For instance, you were chosen as an owner of components/captive_portal,
> content/browser/loader and net, but I'll be happy if you look at the other
> changes.
> 
> The rest are:
> 
> ricea@ - net/websockets
> 
> thakis@ - rlz
> 
> bengr@ - components/precache
> 
> benjhayden@ - download extension API and content
> 
> jam@ - content as a whole (content/child, content/browser/renderer_host)

[baranovich, 2014-12-10 22:24:38]

https://codereview.chromium.org/786423002/diff/20001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/786423002/diff/20001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.cc:1221:
IsResourceTypeFrame(request_data.resource_type);
On 2014/12/10 19:28:30, mmenke wrote:
> Suggest moving the load_flags and is_sync_load stuff down here, too, to keep
> related code together.  Also, the DCHECKs on what flags are set should
probably
> be just before the SetLoadFlags line, now that we're modifying flags here,
too.

Done.

https://codereview.chromium.org/786423002/diff/20001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.cc:1224:
request_data.resource_type == RESOURCE_TYPE_FAVICON;
On 2014/12/10 19:28:30, mmenke wrote:
> BUG:  You need request_data.do_not_prompt_for_login in the mix, too.  The fact
> this didn't make any tests fail is a bit concerning.
> 
> I think this may be a little clearer, than just adding another || clause:
> 
> bool do_not_prompt_for_login = request_data.do_not_prompt_for_login;
> if (... || ...)
>   do_not_prompt_for_login = true;

Done.

https://codereview.chromium.org/786423002/diff/20001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.cc:1224:
request_data.resource_type == RESOURCE_TYPE_FAVICON;
On 2014/12/10 19:28:30, mmenke wrote:
> BUG:  You need request_data.do_not_prompt_for_login in the mix, too.  The fact
> this didn't make any tests fail is a bit concerning.
I didn't found any tests (at least in RDH browsertests/unittests) on that
prompting/non-prompting issue btw.

https://codereview.chromium.org/786423002/diff/20001/content/browser/loader/r...
File content/browser/loader/resource_request_info_impl.cc (right):

https://codereview.chromium.org/786423002/diff/20001/content/browser/loader/r...
content/browser/loader/resource_request_info_impl.cc:66:
blink::WebPageVisibilityStateVisible,    // visibility_state
On 2014/12/10 17:53:25, jam wrote:
> ?

Done.

https://codereview.chromium.org/786423002/diff/20001/content/public/browser/d...
File content/public/browser/download_url_parameters.h (right):

https://codereview.chromium.org/786423002/diff/20001/content/public/browser/d...
content/public/browser/download_url_parameters.h:104: void
set_do_not_prompt_for_login(bool do_not_prompt) {
On 2014/12/10 17:53:25, jam wrote:
> On 2014/12/09 23:33:49, baranovich wrote:
> > If this method is added then load_flags/set_load_flags are not necesssary.
> > Should I delete them or leave for the future?
> 
> remove them

Done.

[bengr, 2014-12-10 23:50:51]

https://codereview.chromium.org/786423002/diff/60001/components/precache/core...
File components/precache/core/precache_fetcher.cc (left):

https://codereview.chromium.org/786423002/diff/60001/components/precache/core...
components/precache/core/precache_fetcher.cc:124:
url_fetcher_->SetLoadFlags(net::LOAD_DO_NOT_PROMPT_FOR_LOGIN);
mmenke@ suggested that we add this in the first place, so please double check
with him that it can be removed.

[Adam Rice, 2014-12-11 09:31:14]

lgtm for net/websockets/

[mmenke, 2014-12-12 19:01:12]

On 2014/12/11 09:31:14, Adam Rice wrote:
> lgtm for net/websockets/

LGTM for my parts

[baranovich, 2014-12-14 13:12:22]

On 2014/12/12 19:01:12, mmenke wrote:
> On 2014/12/11 09:31:14, Adam Rice wrote:
> > lgtm for net/websockets/
> 
> LGTM for my parts

Matt, could you plz confirm to bengr@ that this flag is safe to remove?

[mmenke, 2014-12-15 15:28:16]

On 2014/12/14 13:12:22, baranovich wrote:
> On 2014/12/12 19:01:12, mmenke wrote:
> > On 2014/12/11 09:31:14, Adam Rice wrote:
> > > lgtm for net/websockets/
> > 
> > LGTM for my parts
> 
> Matt, could you plz confirm to bengr@ that this flag is safe to remove?

bengr:  The flag is not needed.  It only mattered for requests that go through
the ResourceDispatcherHost, which yours don't.  This fact wasn't documented in
net/, or even used by net/, so I was unaware of that fact.  That's also one of
the reasons it should be removed.

[bengr, 2014-12-15 16:57:20]

lgtm

[jam, 2014-12-15 19:12:55]

lgtm

[baranovich, 2014-12-17 14:19:23]

thakis, benjhayden, PTAL

[Nico, 2014-12-17 19:01:44]

rlz/ lgtm

[Nico, 2014-12-17 19:02:28]

(and it'd super useful if you tell everyone what you want them to look at, like
mentioned by someone else above already)

[baranovich, 2014-12-24 16:37:14]

baranovich@yandex-team.ru changed reviewers:
+ rdevlin.cronin@chromium.org
- benjhayden@chromium.org

[baranovich, 2014-12-24 16:38:42]

Cannot reach benjhayden :(

rdevlin.cronin@ PTAL at chrome/browser/extensions/api/downloads/downloads_api.cc

[Devlin, 2015-01-05 17:16:41]

On 2014/12/24 16:38:42, baranovich wrote:
> Cannot reach benjhayden :(
> 
> rdevlin.cronin@ PTAL at
chrome/browser/extensions/api/downloads/downloads_api.cc

extensions/api/ lgtm (I trust that downloads owners have ensured that
set_do_not_prompt_for_login() is equivalent to
set_load_flags(DO_NOT_PROMPT_FOR_LOGIN)).

[benjhayden, 2015-01-07 00:52:14]

benjhayden@chromium.org changed reviewers:
+ benjhayden@chromium.org

[benjhayden, 2015-01-07 00:52:15]

Sorry, 2 weeks off hit my inbox hard.
downloads parts look fine as long as somebody (jam?) looked specifically at how
the new flag is used by RDH.

https://codereview.chromium.org/786423002/diff/60001/net/base/load_flags_list.h
File net/base/load_flags_list.h (right):

https://codereview.chromium.org/786423002/diff/60001/net/base/load_flags_list...
net/base/load_flags_list.h:107: LOAD_FLAG(MAYBE_USER_GESTURE, 1 << 23)
Are there any UMA labels or other persistent databases containing these
constants that would be confused by renaming them like this?

[mmenke, 2015-01-08 22:42:09]

https://codereview.chromium.org/786423002/diff/60001/net/base/load_flags_list.h
File net/base/load_flags_list.h (right):

https://codereview.chromium.org/786423002/diff/60001/net/base/load_flags_list...
net/base/load_flags_list.h:107: LOAD_FLAG(MAYBE_USER_GESTURE, 1 << 23)
On 2015/01/07 00:52:15, benjhayden_chromium wrote:
> Are there any UMA labels or other persistent databases containing these
> constants that would be confused by renaming them like this?

No, there aren't.

[benjhayden, 2015-01-08 22:49:42]

On 2015/01/08 22:42:09, mmenke wrote:
>
https://codereview.chromium.org/786423002/diff/60001/net/base/load_flags_list.h
> File net/base/load_flags_list.h (right):
> 
>
https://codereview.chromium.org/786423002/diff/60001/net/base/load_flags_list...
> net/base/load_flags_list.h:107: LOAD_FLAG(MAYBE_USER_GESTURE, 1 << 23)
> On 2015/01/07 00:52:15, benjhayden_chromium wrote:
> > Are there any UMA labels or other persistent databases containing these
> > constants that would be confused by renaming them like this?
> 
> No, there aren't.

Thanks, then LGTM as long as somebody familiar with RDH looked at it.

[baranovich, 2015-01-13 12:05:27]

The CQ bit was checked by baranovich@yandex-team.ru

[commit-bot: I haz the power, 2015-01-13 12:05:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/786423002/60001

[commit-bot: I haz the power, 2015-01-13 12:11:45]

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

[commit-bot: I haz the power, 2015-01-13 12:11:46]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[baranovich, 2015-01-13 12:36:49]

baranovich@yandex-team.ru changed reviewers:
+ jschuh@chromium.org

[baranovich, 2015-01-13 12:37:26]

jshuh@ PTAL at content/common/resource_messages.h

[jschuh, 2015-01-14 14:59:21]

ipc security lgtm. notes: converting a flag to a bool

[baranovich, 2015-01-14 18:37:51]

The CQ bit was checked by baranovich@yandex-team.ru

[commit-bot: I haz the power, 2015-01-14 18:38:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/786423002/60001

[mmenke, 2015-01-14 18:41:47]

On 2015/01/14 18:38:45, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/786423002/60001

Looks like someone else started using the flag in the meantime.  :(

[commit-bot: I haz the power, 2015-01-14 18:50:00]

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

[commit-bot: I haz the power, 2015-01-14 18:50:01]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[baranovich, 2015-01-14 21:31:43]

baranovich@yandex-team.ru changed reviewers:
+ mkwst@chromium.org

[baranovich, 2015-01-14 21:34:44]

mkwst@ PTAL at components/password_manager/core/browser/affiliation_fetcher.cc

This flag does nothing for the URLFetcher and only used in webpage resource
requests. In this CL I try to get rid of it:)

[Mike West, 2015-01-15 04:57:14]

LGTM.

[baranovich, 2015-01-15 10:15:37]

The CQ bit was checked by baranovich@yandex-team.ru

[commit-bot: I haz the power, 2015-01-15 10:16:35]

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

[commit-bot: I haz the power, 2015-01-15 10:30:11]

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

[commit-bot: I haz the power, 2015-01-15 10:30:12]

Try jobs failed on following builders:
  ios_rel_device_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)

[baranovich, 2015-01-15 11:57:08]

baranovich@yandex-team.ru changed reviewers:
+ droger@chromium.org

[baranovich, 2015-01-15 11:58:21]

Totally missed iOS code =\

droger@ PTAL at ios/chrome/browser/net/image_fetcher.mm

[droger, 2015-01-15 12:02:14]

ios/chrome/browser/net/image_fetcher.mm LGTM

[baranovich, 2015-01-15 13:49:37]

The CQ bit was checked by baranovich@yandex-team.ru

[commit-bot: I haz the power, 2015-01-15 13:49:48]

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

[commit-bot: I haz the power, 2015-01-15 14:45:21]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-01-15 14:46:05]

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