Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10880074:
Don't prompt for directory on web intents downloads; handle in current tab. (Closed)
|
Created:
8 years, 3 months ago by Greg Billock Modified:
8 years, 3 months ago CC:
chromium-reviews, rdsmith+dwatch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDon't prompt for directory on web intents downloads; handle in current tab.
R=rdsmith@chromium.org,asanka@chromium.org
BUG=129784, 130015, 141200
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156420
Patch Set 1 #
Total comments: 2
Patch Set 2 : Check dispatcher #
Total comments: 7
Patch Set 3 : Add delivery browser check #
Total comments: 2
Patch Set 4 : Fix condition #Patch Set 5 : Merge to head #
Total comments: 1
Patch Set 6 : Switch to DCHECK #Patch Set 7 : Add target disposition check. #Patch Set 8 : Take out redundant check. #Patch Set 9 : Fix constant ref #Patch Set 10 : ifdef out android #Patch Set 11 : More android ifdef #
Messages
Total messages: 33 (0 generated)
FYI, Asanka is on paternity leave for the week or so.
Can you write a test? http://codereview.chromium.org/10880074/diff/1/chrome/browser/download/chrome... File chrome/browser/download/chrome_download_manager_delegate.cc (right): http://codereview.chromium.org/10880074/diff/1/chrome/browser/download/chrome... chrome/browser/download/chrome_download_manager_delegate.cc:442: delegate = web_intents::GetBrowserForBackgroundWebIntentDelivery( I see GetBrowserForBackgroundWebIntentDelivery uses BrowserList::GetLastActive(), and bails if the profiles don't match. Is there a reason that it doesn't use browser::FindLastActiveWithProfile() instead? I don't know these Browser discovery tools, and I have a vague memory that it is in general frowned upon, but it looks at first glance like FindLastActiveWithProfile() might have a better chance of finding a relevant Browser than GetLastActive(). http://codereview.chromium.org/10880074/diff/1/chrome/browser/download/chrome... chrome/browser/download/chrome_download_manager_delegate.cc:445: delegate->WebIntentDispatch(NULL, dispatcher); GetBrowserForBackgroundWebIntentDelivery may return NULL, so please guard against delegate being NULL.
On 2012/08/27 20:45:58, benjhayden_chromium wrote: > Can you write a test? This is dependent on https://chromiumcodereview.appspot.com/10836164/. I'll add the test after that merges since it'll use some of the same fixture. > > http://codereview.chromium.org/10880074/diff/1/chrome/browser/download/chrome... > File chrome/browser/download/chrome_download_manager_delegate.cc (right): > > http://codereview.chromium.org/10880074/diff/1/chrome/browser/download/chrome... > chrome/browser/download/chrome_download_manager_delegate.cc:442: delegate = > web_intents::GetBrowserForBackgroundWebIntentDelivery( > I see GetBrowserForBackgroundWebIntentDelivery uses > BrowserList::GetLastActive(), and bails if the profiles don't match. > Is there a reason that it doesn't use browser::FindLastActiveWithProfile() > instead? > I don't know these Browser discovery tools, and I have a vague memory that it is > in general frowned upon, but it looks at first glance like > FindLastActiveWithProfile() might have a better chance of finding a relevant > Browser than GetLastActive(). Yes. I have a friend exception for GetLastActive for that code. http://codereview.chromium.org/10880074/diff/1/chrome/browser/download/chrome... > chrome/browser/download/chrome_download_manager_delegate.cc:445: > delegate->WebIntentDispatch(NULL, dispatcher); > GetBrowserForBackgroundWebIntentDelivery may return NULL, so please guard > against delegate being NULL. Good point. Done.
On Tue, Aug 28, 2012 at 11:53 AM, <gbillock@chromium.org> wrote: > On 2012/08/27 20:45:58, benjhayden_chromium wrote: > >> Can you write a test? >> > > This is dependent on https://chromiumcodereview.**appspot.com/10836164/<https://chromiumcodereview.... > I'll add > the test after that merges since it'll use some of the same fixture. > > > > > http://codereview.chromium.**org/10880074/diff/1/chrome/** > browser/download/chrome_**download_manager_delegate.cc<http://codereview.chromium.org/10880074/diff/1/chrome/browser/download/chrome_download_manager_delegate.cc> > >> File chrome/browser/download/**chrome_download_manager_**delegate.cc >> (right): >> > > > http://codereview.chromium.**org/10880074/diff/1/chrome/** > browser/download/chrome_**download_manager_delegate.cc#**newcode442<http://codereview.chromium.org/10880074/diff/1/chrome/browser/download/chrome_download_manager_delegate.cc#newcode442> > >> chrome/browser/download/**chrome_download_manager_**delegate.cc:442: >> delegate = >> web_intents::**GetBrowserForBackgroundWebInte**ntDelivery( >> I see GetBrowserForBackgroundWebInte**ntDelivery uses >> BrowserList::GetLastActive(), and bails if the profiles don't match. >> Is there a reason that it doesn't use browser::** >> FindLastActiveWithProfile() >> instead? >> I don't know these Browser discovery tools, and I have a vague memory >> that it >> > is > >> in general frowned upon, but it looks at first glance like >> FindLastActiveWithProfile() might have a better chance of finding a >> relevant >> Browser than GetLastActive(). >> > > Yes. I have a friend exception for GetLastActive for that code. Great, that takes care of my concern that it's frowned on. However, I still have a question. I'll try to ask my question more clearly. Wouldn't FindLastActiveWithProfile() have a better chance of finding a relevant Browser than GetLastActive()? Why use GetLastActive instead of FLAWP? > > > http://codereview.chromium.**org/10880074/diff/1/chrome/** > browser/download/chrome_**download_manager_delegate.cc#**newcode445<http://codereview.chromium.org/10880074/diff/1/chrome/browser/download/chrome_download_manager_delegate.cc#newcode445> > >> chrome/browser/download/**chrome_download_manager_**delegate.cc:445: >> delegate->WebIntentDispatch(**NULL, dispatcher); >> GetBrowserForBackgroundWebInte**ntDelivery may return NULL, so please >> guard >> against delegate being NULL. >> > > Good point. Done. > > http://codereview.chromium.**org/10880074/<http://codereview.chromium.org/108... >
http://codereview.chromium.org/10880074/diff/7001/chrome/browser/download/chr... File chrome/browser/download/chrome_download_manager_delegate.cc (right): http://codereview.chromium.org/10880074/diff/7001/chrome/browser/download/chr... chrome/browser/download/chrome_download_manager_delegate.cc:446: delegate->WebIntentDispatch(NULL, dispatcher); Shouldn't this logic be duplicated in ShouldOpenWithWebIntents so that we don't say "Yes, we'll open it with web intents!" and then don't find a delegate and skip doing so? http://codereview.chromium.org/10880074/diff/7001/chrome/browser/download/chr... chrome/browser/download/chrome_download_manager_delegate.cc:716: if (ShouldOpenWithWebIntents(download)) Help me understand the reason for this? I'm worried, because I think this is the only signal we currently have that we're doing a "Save As ..." download, and we want that to bypass web intents.
On Tue, Aug 28, 2012 at 9:34 AM, Ben Hayden <benjhayden@chromium.org> wrote: > > > On Tue, Aug 28, 2012 at 11:53 AM, <gbillock@chromium.org> wrote: >> >> On 2012/08/27 20:45:58, benjhayden_chromium wrote: >>> >>> Can you write a test? >> >> >> This is dependent on https://chromiumcodereview.appspot.com/10836164/. >> I'll add >> the test after that merges since it'll use some of the same fixture. >> >> >> >> >> >> http://codereview.chromium.org/10880074/diff/1/chrome/browser/download/chrome... >>> >>> File chrome/browser/download/chrome_download_manager_delegate.cc (right): >> >> >> >> >> http://codereview.chromium.org/10880074/diff/1/chrome/browser/download/chrome... >>> >>> chrome/browser/download/chrome_download_manager_delegate.cc:442: delegate >>> = >>> web_intents::GetBrowserForBackgroundWebIntentDelivery( >>> I see GetBrowserForBackgroundWebIntentDelivery uses >>> BrowserList::GetLastActive(), and bails if the profiles don't match. >>> Is there a reason that it doesn't use >>> browser::FindLastActiveWithProfile() >>> instead? >>> I don't know these Browser discovery tools, and I have a vague memory >>> that it >> >> is >>> >>> in general frowned upon, but it looks at first glance like >>> FindLastActiveWithProfile() might have a better chance of finding a >>> relevant >>> Browser than GetLastActive(). >> >> >> Yes. I have a friend exception for GetLastActive for that code. > > > Great, that takes care of my concern that it's frowned on. > > However, I still have a question. I'll try to ask my question more clearly. > > Wouldn't FindLastActiveWithProfile() have a better chance of finding a > relevant Browser than GetLastActive()? > > Why use GetLastActive instead of FLAWP? Ah, I see. Yes, it would be better here. Here's the tradeoff. The explicit web-intents method is used for system events as well, and so needs to be able to find an active browser. So supporting that is more likely to be stable as BrowserFinder functionality is discouraged. So re-using that special-purpose method looks less frowned-upon than using FLAWP directly. It's a bit of a close judgment, though, so if I got it wrong, I'm happy to change it. >> http://codereview.chromium.org/10880074/diff/1/chrome/browser/download/chrome... >>> >>> chrome/browser/download/chrome_download_manager_delegate.cc:445: >>> delegate->WebIntentDispatch(NULL, dispatcher); >>> GetBrowserForBackgroundWebIntentDelivery may return NULL, so please guard >>> against delegate being NULL. >> >> >> Good point. Done. >> >> http://codereview.chromium.org/10880074/ > >
http://codereview.chromium.org/10880074/diff/7001/chrome/browser/download/chr... File chrome/browser/download/chrome_download_manager_delegate.cc (right): http://codereview.chromium.org/10880074/diff/7001/chrome/browser/download/chr... chrome/browser/download/chrome_download_manager_delegate.cc:446: delegate->WebIntentDispatch(NULL, dispatcher); What I found in my testing is that for target=_blank cases is that the item- >GetWebContents() starts out valid but then goes invalid. I think this happens at a predictable place, but I'll add the GetBrowserForBackgroundWebIntentDelivery call to make sure the fallback will complete. On 2012/08/28 18:03:43, rdsmith wrote: > Shouldn't this logic be duplicated in ShouldOpenWithWebIntents so that we don't > say "Yes, we'll open it with web intents!" and then don't find a delegate and > skip doing so? http://codereview.chromium.org/10880074/diff/7001/chrome/browser/download/chr... chrome/browser/download/chrome_download_manager_delegate.cc:716: if (ShouldOpenWithWebIntents(download)) Yes, we definitely do. What I'm trying to do here is suppress the "choose a directory for this file" which cripples the use case. We definitely want Save As... to skip WI though. Is there a condition I can add to ShouldOpenWithWebIntents that detects Save As? That'd be a good fix to include here. On 2012/08/28 18:03:43, rdsmith wrote: > Help me understand the reason for this? I'm worried, because I think this is > the only signal we currently have that we're doing a "Save As ..." download, and > we want that to bypass web intents.
http://codereview.chromium.org/10880074/diff/7001/chrome/browser/download/chr... File chrome/browser/download/chrome_download_manager_delegate.cc (right): http://codereview.chromium.org/10880074/diff/7001/chrome/browser/download/chr... chrome/browser/download/chrome_download_manager_delegate.cc:716: if (ShouldOpenWithWebIntents(download)) On 2012/08/28 18:03:43, rdsmith wrote: > Help me understand the reason for this? I'm worried, because I think this is > the only signal we currently have that we're doing a "Save As ..." download, and > we want that to bypass web intents. Can you show how Save Pages use this code path? I couldn't see, so I stuck a print statement in here and saved a page and did not see the print statement, but I might be missing something.
On 2012/08/28 18:42:14, benjhayden_chromium wrote: > http://codereview.chromium.org/10880074/diff/7001/chrome/browser/download/chr... > File chrome/browser/download/chrome_download_manager_delegate.cc (right): > > http://codereview.chromium.org/10880074/diff/7001/chrome/browser/download/chr... > chrome/browser/download/chrome_download_manager_delegate.cc:716: if > (ShouldOpenWithWebIntents(download)) > On 2012/08/28 18:03:43, rdsmith wrote: > > Help me understand the reason for this? I'm worried, because I think this is > > the only signal we currently have that we're doing a "Save As ..." download, > and > > we want that to bypass web intents. > > Can you show how Save Pages use this code path? > I couldn't see, so I stuck a print statement in here and saved a page and did > not see the print statement, but I might be missing something. Sorry, "Save * As ...", e.g. save link as, save image as.
http://codereview.chromium.org/10880074/diff/7001/chrome/browser/download/chr... File chrome/browser/download/chrome_download_manager_delegate.cc (right): http://codereview.chromium.org/10880074/diff/7001/chrome/browser/download/chr... chrome/browser/download/chrome_download_manager_delegate.cc:716: if (ShouldOpenWithWebIntents(download)) On 2012/08/28 18:38:00, Greg Billock wrote: > Yes, we definitely do. What I'm trying to do here is suppress the "choose a > directory for this file" which cripples the use case. We definitely want Save > As... to skip WI though. Is there a condition I can add to > ShouldOpenWithWebIntents that detects Save As? That'd be a good fix to include > here. > > On 2012/08/28 18:03:43, rdsmith wrote: > > Help me understand the reason for this? I'm worried, because I think this is > > the only signal we currently have that we're doing a "Save As ..." download, > and > > we want that to bypass web intents. > I don't completely understand the use case--do you know what triggers the selecting of a directory for that file? I think if the download has TARGET_DISPOSITION_PROMPT that means that either a) the download was triggered by an extension that requested prompting, b) It's from a context menu, c) It's from a "Save Page As ..." that couldn't use the regular pathway, or d) It's from a renderer plugin requesting save. All of those are cases I think it's fine to disable web intents for, but I'm not sure why else should_prompt would be true here except for those cases. Can you give me a sense as to how should_prompt ends up true here in the case you're trying to deal with? (Pretty much the same question as above.)
On 2012/08/28 18:29:41, Greg Billock wrote: > On Tue, Aug 28, 2012 at 9:34 AM, Ben Hayden <mailto:benjhayden@chromium.org> wrote: > > > > > > On Tue, Aug 28, 2012 at 11:53 AM, <mailto:gbillock@chromium.org> wrote: > >> > >> On 2012/08/27 20:45:58, benjhayden_chromium wrote: > >>> > >>> Can you write a test? > >> > >> > >> This is dependent on https://chromiumcodereview.appspot.com/10836164/. > >> I'll add > >> the test after that merges since it'll use some of the same fixture. > >> > >> > >> > >> > >> > >> > http://codereview.chromium.org/10880074/diff/1/chrome/browser/download/chrome... > >>> > >>> File chrome/browser/download/chrome_download_manager_delegate.cc (right): > >> > >> > >> > >> > >> > http://codereview.chromium.org/10880074/diff/1/chrome/browser/download/chrome... > >>> > >>> chrome/browser/download/chrome_download_manager_delegate.cc:442: delegate > >>> = > >>> web_intents::GetBrowserForBackgroundWebIntentDelivery( > >>> I see GetBrowserForBackgroundWebIntentDelivery uses > >>> BrowserList::GetLastActive(), and bails if the profiles don't match. > >>> Is there a reason that it doesn't use > >>> browser::FindLastActiveWithProfile() > >>> instead? > >>> I don't know these Browser discovery tools, and I have a vague memory > >>> that it > >> > >> is > >>> > >>> in general frowned upon, but it looks at first glance like > >>> FindLastActiveWithProfile() might have a better chance of finding a > >>> relevant > >>> Browser than GetLastActive(). > >> > >> > >> Yes. I have a friend exception for GetLastActive for that code. > > > > > > Great, that takes care of my concern that it's frowned on. > > > > However, I still have a question. I'll try to ask my question more clearly. > > > > Wouldn't FindLastActiveWithProfile() have a better chance of finding a > > relevant Browser than GetLastActive()? > > > > Why use GetLastActive instead of FLAWP? > > Ah, I see. Yes, it would be better here. Here's the tradeoff. The > explicit web-intents method is used for system events as well, and so > needs to be able to find an active browser. So supporting that is more > likely to be stable as BrowserFinder functionality is discouraged. So > re-using that special-purpose method looks less frowned-upon than > using FLAWP directly. It's a bit of a close judgment, though, so if I > got it wrong, I'm happy to change it. I don't grok BrowserFinder, so I'm not the one who knows which it should be. Ben Goodger ben@chromium.org and John Abd-El-Malek jam@chromium.org would know. Can you ask one of them for a recommendation between FLAWP and GetLastActive? You might also want to search your gmail for "PSA: Many uses of Browser/BrowserList considered harmful" from Ben Goodger on 21 May 2012. I can forward it to you if you can't find it. Now that I re-read that thread, I wonder why WebIntentDispatch() is in WebContentsDelegate instead of a ProfileKeyedService? Anyway, I won't hold this CL for these questions, but please consider using a ProfileKeyedService and/or FLAWP. > > >> > http://codereview.chromium.org/10880074/diff/1/chrome/browser/download/chrome... > >>> > >>> chrome/browser/download/chrome_download_manager_delegate.cc:445: > >>> delegate->WebIntentDispatch(NULL, dispatcher); > >>> GetBrowserForBackgroundWebIntentDelivery may return NULL, so please guard > >>> against delegate being NULL. > >> > >> > >> Good point. Done. > >> > >> http://codereview.chromium.org/10880074/ > > > >
LGTM, please wait for Randy. http://codereview.chromium.org/10880074/diff/9002/chrome/browser/download/chr... File chrome/browser/download/chrome_download_manager_delegate.cc (right): http://codereview.chromium.org/10880074/diff/9002/chrome/browser/download/chr... chrome/browser/download/chrome_download_manager_delegate.cc:373: if (web_intents::GetBrowserForBackgroundWebIntentDelivery(profile_) == NULL) if (!web_intents::...(...)) instead of explicit comparison with NULL.
On 2012/08/28 19:13:06, benjhayden_chromium wrote: > LGTM, please wait for Randy. > > http://codereview.chromium.org/10880074/diff/9002/chrome/browser/download/chr... > File chrome/browser/download/chrome_download_manager_delegate.cc (right): > > http://codereview.chromium.org/10880074/diff/9002/chrome/browser/download/chr... > chrome/browser/download/chrome_download_manager_delegate.cc:373: if > (web_intents::GetBrowserForBackgroundWebIntentDelivery(profile_) == NULL) > if (!web_intents::...(...)) instead of explicit comparison with NULL. What's the status of this CL? Are you waiting for me (in which case, my abject apologies--feel free to ping me if you need something from me on a review and it's been more than 24 hours) or are you planning to upload a new patch set? Looking over this CL again, I'm disturbed by adding dependencies on BrowserList, which I thought was being deprecated. Are you sure that's a reasonable thing to do?
http://codereview.chromium.org/10880074/diff/9002/chrome/browser/download/chr... File chrome/browser/download/chrome_download_manager_delegate.cc (right): http://codereview.chromium.org/10880074/diff/9002/chrome/browser/download/chr... chrome/browser/download/chrome_download_manager_delegate.cc:373: if (web_intents::GetBrowserForBackgroundWebIntentDelivery(profile_) == NULL) On 2012/08/28 19:13:06, benjhayden_chromium wrote: > if (!web_intents::...(...)) instead of explicit comparison with NULL. Done.
On 2012/09/07 18:52:34, Greg Billock wrote: > http://codereview.chromium.org/10880074/diff/9002/chrome/browser/download/chr... > File chrome/browser/download/chrome_download_manager_delegate.cc (right): > > http://codereview.chromium.org/10880074/diff/9002/chrome/browser/download/chr... > chrome/browser/download/chrome_download_manager_delegate.cc:373: if > (web_intents::GetBrowserForBackgroundWebIntentDelivery(profile_) == NULL) > On 2012/08/28 19:13:06, benjhayden_chromium wrote: > > if (!web_intents::...(...)) instead of explicit comparison with NULL. > > Done. OK, Randy, This is now merged to base on the previous CL and is ready for review.
In skimming the old review comments, I didn't see a response from you either to my query about overriding the should_prompt value, or to my query about relying on BrowserList::GetLastActive(). Any thoughts on those two issues? (See earlier in the stream for more details.) http://codereview.chromium.org/10880074/diff/22001/chrome/browser/download/ch... File chrome/browser/download/chrome_download_manager_delegate.cc (right): http://codereview.chromium.org/10880074/diff/22001/chrome/browser/download/ch... chrome/browser/download/chrome_download_manager_delegate.cc:475: delegate->WebIntentDispatch(NULL, dispatcher); DCHECK opportunity here? Should we ever end up here with delegate NULL, given ShouldOpenWithWebIntents?
On 2012/09/10 15:43:35, rdsmith wrote: > In skimming the old review comments, I didn't see a response from you either to > my query about overriding the should_prompt value, See #8. I think you are right, I'm just not sure what to add to the conditional. or to my query about relying > on BrowserList::GetLastActive(). Any thoughts on those two issues? (See > earlier in the stream for more details.) Maybe this discussion was in a non-review thread? There's a web intents exception for GetLastActive (essentially), which we're using here. The rationale for that is not just this case -- there are other browser-initiated intents that need to find the last active browser, so we'll continue maintaining that web_intents_util method for the foreseeable future. > http://codereview.chromium.org/10880074/diff/22001/chrome/browser/download/ch... > File chrome/browser/download/chrome_download_manager_delegate.cc (right): > > http://codereview.chromium.org/10880074/diff/22001/chrome/browser/download/ch... > chrome/browser/download/chrome_download_manager_delegate.cc:475: > delegate->WebIntentDispatch(NULL, dispatcher); > DCHECK opportunity here? Should we ever end up here with delegate NULL, given > ShouldOpenWithWebIntents? Yeah, this is defensive. I can switch to DCHECK.
On 2012/09/10 16:08:58, Greg Billock wrote: > On 2012/09/10 15:43:35, rdsmith wrote: > > In skimming the old review comments, I didn't see a response from you either > to > > my query about overriding the should_prompt value, > > See #8. I think you are right, I'm just not sure what to add to the conditional. See #11. I don't understand why should_prompt is ending up true at the point where you override--by my read of the code it shouldn't in any case in which we want to use web intents. Can you give me a repro that results in a prompt in a web intent case? I can probably figure out the right conditional then. > or to my query about relying > > on BrowserList::GetLastActive(). Any thoughts on those two issues? (See > > earlier in the stream for more details.) > > Maybe this discussion was in a non-review thread? There's a web intents > exception for GetLastActive (essentially), which we're using here. The rationale > for that is not just this case -- there are other browser-initiated intents that > need to find the last active browser, so we'll continue maintaining that > web_intents_util method for the foreseeable future. Ok, SG. > > > > http://codereview.chromium.org/10880074/diff/22001/chrome/browser/download/ch... > > File chrome/browser/download/chrome_download_manager_delegate.cc (right): > > > > > http://codereview.chromium.org/10880074/diff/22001/chrome/browser/download/ch... > > chrome/browser/download/chrome_download_manager_delegate.cc:475: > > delegate->WebIntentDispatch(NULL, dispatcher); > > DCHECK opportunity here? Should we ever end up here with delegate NULL, given > > ShouldOpenWithWebIntents? > > Yeah, this is defensive. I can switch to DCHECK.
http://codereview.chromium.org/10880074/diff/7001/chrome/browser/download/chr... File chrome/browser/download/chrome_download_manager_delegate.cc (right): http://codereview.chromium.org/10880074/diff/7001/chrome/browser/download/chr... chrome/browser/download/chrome_download_manager_delegate.cc:716: if (ShouldOpenWithWebIntents(download)) Ah! I somehow didn't see this #11 in the thread. I've added the TARGET_DISPOSITION_PROMPT check to ShouldOpenWithWebIntents, which we definitely want to do. Does that mean that this condition should stay? I'd like to suppress the choose-the-path dialog, which the comments in 719+ makes it look like get established here. On 2012/08/28 19:05:19, rdsmith wrote: > On 2012/08/28 18:38:00, Greg Billock wrote: > > Yes, we definitely do. What I'm trying to do here is suppress the "choose a > > directory for this file" which cripples the use case. We definitely want Save > > As... to skip WI though. Is there a condition I can add to > > ShouldOpenWithWebIntents that detects Save As? That'd be a good fix to include > > here. > > > > On 2012/08/28 18:03:43, rdsmith wrote: > > > Help me understand the reason for this? I'm worried, because I think this > is > > > the only signal we currently have that we're doing a "Save As ..." download, > > and > > > we want that to bypass web intents. > > > > I don't completely understand the use case--do you know what triggers the > selecting of a directory for that file? > > I think if the download has TARGET_DISPOSITION_PROMPT that means that either a) > the download was triggered by an extension that requested prompting, b) It's > from a context menu, c) It's from a "Save Page As ..." that couldn't use the > regular pathway, or d) It's from a renderer plugin requesting save. All of > those are cases I think it's fine to disable web intents for, but I'm not sure > why else should_prompt would be true here except for those cases. Can you give > me a sense as to how should_prompt ends up true here in the case you're trying > to deal with? (Pretty much the same question as above.)
LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/10880074/23003
Try job failure for 10880074-23003 (retry) on android for steps "compile, build" (clobber build). It's a second try, previously, steps "compile, build" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/10880074/23003
Try job failure for 10880074-23003 (retry) on android for steps "compile, build" (clobber build). It's a second try, previously, steps "compile, build" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/10880074/31005
Try job failure for 10880074-31005 (retry) on android for steps "compile, build" (clobber build). It's a second try, previously, steps "compile, build" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/10880074/31005
Try job failure for 10880074-31005 (retry) on android for steps "compile, build" (clobber build). It's a second try, previously, steps "compile, build" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/10880074/26005
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/10880074/26005
Change committed as 156420 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
