Use suggested filename for "Save Link As"

Use suggested filename for "Save Link As"

Use filename mentioned in download attribute of anchor element while saving file using "Save Link As" from context menu.

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

[Nikhil, 2014-06-17 09:36:50]

Chrome side changes to use suggested filename when saving the file. PTAL.
Thanks!

[kbalazs, 2014-06-18 21:38:32]

On 2014/06/17 09:36:50, Nikhil wrote:
> Chrome side changes to use suggested filename when saving the file. PTAL.
> Thanks!

WIP + PTAL? Not very consistent :)

[Nikhil, 2014-06-19 05:05:12]

On 2014/06/18 21:38:32, kbalazs wrote:
> On 2014/06/17 09:36:50, Nikhil wrote:
> > Chrome side changes to use suggested filename when saving the file. PTAL.
> > Thanks!
> 
> WIP + PTAL? Not very consistent :)

Yea, PTAL mainly to see whether current approach seems okay. I should've
mentioned it :)

WIP since I'm not able to get value in RenderViewContextMenu::ExecuteCommand()
after setting it in ContextMenuParamsBuilder::Build(). Still need to figure it
out. Plus, Blink side changes are still under review.

[Nikhil, 2014-06-19 15:08:59]

Adding more reviewers based on OWNERS file. PTAL.

Also, how do I write test for this? I'm unable to find any reference test for
'Save Link As' option of context menu.

Thanks!

[lazyboy, 2014-06-19 18:09:51]

On 2014/06/19 15:08:59, Nikhil wrote:
> Adding more reviewers based on OWNERS file. PTAL.
> 
> Also, how do I write test for this? I'm unable to find any reference test for
> 'Save Link As' option of context menu.
I'm assuming you want to trigger a context menu for a link and check if
"Save Link As" is there, then click it and check whether suggested filename
got propagated correctly.

So it would be something similar to existing tests:
e.g.: ContextMenuBrowserTest.OpenInNewTabReferrer:

TestRenderViewContextMenu menu(
      browser()->tab_strip_model()->GetActiveWebContents()->GetMainFrame(),
      context_menu_params);
  menu.Init();
  menu.ExecuteCommand(IDC_CONTENT_CONTEXT_SAVELINKAS, ...);

Now ExecuteCommand() would actually trigger a download, which we don't want
for tests, you have search for a bit to see if you can trigger the download on
a mock download manager instead of the real one.
TestBrowserWindow has a TestDownloadShelf, which might be the one to use, but
that's only my speculation.

> 
> Thanks!

[Nikhil, 2014-06-20 13:34:52]

Added test case.

[Nikhil, 2014-06-20 13:45:30]

On 2014/06/19 18:09:51, lazyboy wrote:
> On 2014/06/19 15:08:59, Nikhil wrote:
> > Adding more reviewers based on OWNERS file. PTAL.
> > 
> > Also, how do I write test for this? I'm unable to find any reference test
for
> > 'Save Link As' option of context menu.
> I'm assuming you want to trigger a context menu for a link and check if
> "Save Link As" is there, then click it and check whether suggested filename
> got propagated correctly.
> 
> So it would be something similar to existing tests:
> e.g.: ContextMenuBrowserTest.OpenInNewTabReferrer:
> 
> TestRenderViewContextMenu menu(
>       browser()->tab_strip_model()->GetActiveWebContents()->GetMainFrame(),
>       context_menu_params);
>   menu.Init();
>   menu.ExecuteCommand(IDC_CONTENT_CONTEXT_SAVELINKAS, ...);
> 

Thanks for the inputs! 

I couldn't figure out the correct way to cover this, but I've written one based
on ContextMenuBrowserTest.RealMenu. Please have a look. If there is another way
then we can follow it.

* Ideally, I'd want to get the value when click event is handled and
ContextMenuParams is populated instead of creating one with
TestRenderViewContextMenu. This is because I want to cover Blink side changes as
well with this test.

* The scope of this change is to extract suggested filename and set it when
creating download request. Download module can choose to ignore it based on
origin and other constraints.

Current test case uses following approach:

* Register observers for NOTIFICATION_RENDER_VIEW_CONTEXT_MENU_SHOWN and
NOTIFICATION_DOWNLOAD_INITIATED.
* Navigate to link
* Right click and execute IDC_CONTENT_CONTEXT_SAVELINKAS
* Compare extracted filename to original value

> Now ExecuteCommand() would actually trigger a download, which we don't want
> for tests, you have search for a bit to see if you can trigger the download on
> a mock download manager instead of the real one.
> TestBrowserWindow has a TestDownloadShelf, which might be the one to use, but
> that's only my speculation.
> 
> > 
> > Thanks!

Maybe we don't need to trigger download since we just need to compare filename
value. Download module can choose to ignore this. But, I was unable to avoid it.
The current implementation of ContextMenuNotificationObserver executes command.

[lazyboy, 2014-06-20 17:23:31]

https://chromiumcodereview.appspot.com/339153002/diff/40001/chrome/browser/re...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/339153002/diff/40001/chrome/browser/re...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:235:
ContextMenuNotificationObserver menu_observer(IDC_CONTENT_CONTEXT_SAVELINKAS);
Using ContextMenuNotificationObserver would result in
RVContextMenu::ExecuteCommand to be called which would initiate the download.
You can avoid the download by creating a similar but different
NotificationObserver, that would look for
NOTIFICATION_RENDER_VIEW_CONTEXT_MENU_SHOWN and PostTask a
RenderViewContextMenu::Cancel() and read the params() from the
RenderViewContextMenu*. Then you just verify whether params().suggested_filename
is the right one.

[Nikhil, 2014-06-20 19:37:52]

https://codereview.chromium.org/339153002/diff/40001/chrome/browser/renderer_...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/339153002/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:235:
ContextMenuNotificationObserver menu_observer(IDC_CONTENT_CONTEXT_SAVELINKAS);
I tried similar approach earlier by modifying existing
ContextMenuNotificationObserver. One problem that I faced was I still needed
"Wait" mechanism for context menu to get created after mouse event. But I think
I can manage it somehow and remove observer for NOTIFICATION_DOWNLOAD_INITIATED.

I'll try to prepare a patch for this.

On 2014/06/20 17:23:31, lazyboy wrote:
> Using ContextMenuNotificationObserver would result in
> RVContextMenu::ExecuteCommand to be called which would initiate the download.
> You can avoid the download by creating a similar but different
> NotificationObserver, that would look for
> NOTIFICATION_RENDER_VIEW_CONTEXT_MENU_SHOWN and PostTask a
> RenderViewContextMenu::Cancel() and read the params() from the
> RenderViewContextMenu*. Then you just verify whether
params().suggested_filename
> is the right one.

[lazyboy, 2014-06-20 19:50:57]

https://codereview.chromium.org/339153002/diff/40001/chrome/browser/renderer_...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/339153002/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:235:
ContextMenuNotificationObserver menu_observer(IDC_CONTENT_CONTEXT_SAVELINKAS);
On 2014/06/20 19:37:51, Nikhil wrote:
> I tried similar approach earlier by modifying existing
> ContextMenuNotificationObserver. One problem that I faced was I still needed
> "Wait" mechanism for context menu to get created after mouse event.
The notification observer in that class should accomplish exactly that, i.e.
"Wait". The notification is fired once the menu is shown after the event.
 But I think
> I can manage it somehow and remove observer for
NOTIFICATION_DOWNLOAD_INITIATED.
Sure, if you can remove the actual download from the test, that should be good
enough.
> 
> I'll try to prepare a patch for this.
> 
> On 2014/06/20 17:23:31, lazyboy wrote:
> > Using ContextMenuNotificationObserver would result in
> > RVContextMenu::ExecuteCommand to be called which would initiate the
download.
> > You can avoid the download by creating a similar but different
> > NotificationObserver, that would look for
> > NOTIFICATION_RENDER_VIEW_CONTEXT_MENU_SHOWN and PostTask a
> > RenderViewContextMenu::Cancel() and read the params() from the
> > RenderViewContextMenu*. Then you just verify whether
> params().suggested_filename
> > is the right one.
>

[Nikhil, 2014-06-23 08:02:35]

PTAL. Thanks!

https://codereview.chromium.org/339153002/diff/40001/chrome/browser/renderer_...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
(right):

https://codereview.chromium.org/339153002/diff/40001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:235:
ContextMenuNotificationObserver menu_observer(IDC_CONTENT_CONTEXT_SAVELINKAS);
Modified existing ContextMenuNotificationObserver to consider
IDC_CONTENT_CONTEXT_SAVELINKAS and avoid actual download while executing
command. I had to introduce "WaitForMenu()" because test case won't wait even
though notification observer is waiting to be notified for
NOTIFICATION_RENDER_VIEW_CONTEXT_MENU_SHOWN. 

Please see if this approach looks okay. I verified ContextMenuBrowserTest tests
and all passed.

On 2014/06/20 19:50:56, lazyboy wrote:
> On 2014/06/20 19:37:51, Nikhil wrote:
> > I tried similar approach earlier by modifying existing
> > ContextMenuNotificationObserver. One problem that I faced was I still needed
> > "Wait" mechanism for context menu to get created after mouse event.
> The notification observer in that class should accomplish exactly that, i.e.
> "Wait". The notification is fired once the menu is shown after the event.
>  But I think
> > I can manage it somehow and remove observer for
> NOTIFICATION_DOWNLOAD_INITIATED.
> Sure, if you can remove the actual download from the test, that should be good
> enough.
> > 
> > I'll try to prepare a patch for this.
> > 
> > On 2014/06/20 17:23:31, lazyboy wrote:
> > > Using ContextMenuNotificationObserver would result in
> > > RVContextMenu::ExecuteCommand to be called which would initiate the
> download.
> > > You can avoid the download by creating a similar but different
> > > NotificationObserver, that would look for
> > > NOTIFICATION_RENDER_VIEW_CONTEXT_MENU_SHOWN and PostTask a
> > > RenderViewContextMenu::Cancel() and read the params() from the
> > > RenderViewContextMenu*. Then you just verify whether
> > params().suggested_filename
> > > is the right one.
> > 
>

[lazyboy, 2014-06-24 01:15:04]

Couple of comments about the test util.

https://codereview.chromium.org/339153002/diff/60001/chrome/browser/renderer_...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc
(right):

https://codereview.chromium.org/339153002/diff/60001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc:33:
context_menu_ = content::Source<RenderViewContextMenu>(source).ptr();
Instead of saving the raw pointer of RVContextMenu, just copy
context_menu_->params() to a member.
That way you don't have to worry whether context_menu_ is valid ptr anymore or
not (Once you show another context menu, this would become invalid iiuc).

https://codereview.chromium.org/339153002/diff/60001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc:63:
case IDC_CONTENT_CONTEXT_OPENLINKNEWTAB: {
The class is named more generic as ContextMenUNotificationObserver but is doing
very special focused things for NEWTAB and SAVELINKAS.
I'd extract the SaveLinkAs to separate class, possibly extending from
ContextMenuNOtificationObserver, to sth like SaveLinkAsContextMenuObserver or
DownloadContextMenuObserver. Then we can have our own virtual ExecuteCommand
implementation there.
Otherwise this class does not remain generic enough to be used by any test that
requires observer context menu.

[Nikhil, 2014-06-24 10:38:43]

PTAL. I introduced SaveLinkAsContextMenuObserver, and removed the changes from
ContextMenuNotificationObserver.

Thanks!

[Nikhil, 2014-06-24 10:39:52]

https://codereview.chromium.org/339153002/diff/60001/chrome/browser/renderer_...
File
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc
(right):

https://codereview.chromium.org/339153002/diff/60001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc:33:
context_menu_ = content::Source<RenderViewContextMenu>(source).ptr();
On 2014/06/24 01:15:04, lazyboy wrote:
> Instead of saving the raw pointer of RVContextMenu, just copy
> context_menu_->params() to a member.
> That way you don't have to worry whether context_menu_ is valid ptr anymore or
> not (Once you show another context menu, this would become invalid iiuc).

Done.

https://codereview.chromium.org/339153002/diff/60001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc:63:
case IDC_CONTENT_CONTEXT_OPENLINKNEWTAB: {
On 2014/06/24 01:15:04, lazyboy wrote:
> The class is named more generic as ContextMenUNotificationObserver but is
doing
> very special focused things for NEWTAB and SAVELINKAS.
> I'd extract the SaveLinkAs to separate class, possibly extending from
> ContextMenuNOtificationObserver, to sth like SaveLinkAsContextMenuObserver or
> DownloadContextMenuObserver. Then we can have our own virtual ExecuteCommand
> implementation there.
> Otherwise this class does not remain generic enough to be used by any test
that
> requires observer context menu.

Done.

[lazyboy, 2014-06-24 14:25:51]

render_view_context_menu* LGTM.
Thanks for making the changes.

[Nikhil, 2014-06-24 15:05:29]

On 2014/06/24 14:25:51, lazyboy wrote:
> render_view_context_menu* LGTM.
> Thanks for making the changes.

Great! Thanks for reviewing :)

One question, do I need to add OWNERS for context_menu_params* and
frame_messages.h?

[lazyboy, 2014-06-24 15:29:15]

On 2014/06/24 15:05:29, Nikhil wrote:
> On 2014/06/24 14:25:51, lazyboy wrote:
> > render_view_context_menu* LGTM.
> > Thanks for making the changes.
> 
> Great! Thanks for reviewing :)
> 
> One question, do I need to add OWNERS for context_menu_params* and
> frame_messages.h?

You still need OWNERS review for
content/common/frame_messages.h
content/public/common/context_menu_params.h
content/renderer/context_menu_params_builder.cc

[Nikhil, 2014-06-25 06:20:56]

Adding more reviewers for OWNERS review.

@tsepez: OWNER review for content/common/frame_messages.h and
content/public/common/context_menu_params.h please.

@jamesr: OWNER review for content/renderer/context_menu_params_builder.cc
please.

[Tom Sepez, 2014-06-25 16:51:00]

Where do you validate that suggested_filename doesn't contain things like '..'
as part of a path?  Where does this flow to in the browser process?

[jamesr, 2014-06-25 18:16:16]

the /renderer/ side lgtm, but you definitely need to validate this carefully on
the browser side.  The code also appears to blindly attempt a utf16->utf8
conversion, but the value from the renderer could of course be invalid utf16 or
not unicode at all, so you need to validate that as well.

[Nikhil, 2014-06-26 10:44:24]

On 2014/06/25 16:51:00, Tom Sepez wrote:
> Where do you validate that suggested_filename doesn't contain things like '..'
> as part of a path?  Where does this flow to in the browser process?

suggested_filename is populated based on value of download attribute of anchor
element. Here's the link to Blink changes -
https://codereview.chromium.org/334243004/

suggested_filename will be considered when trying to generate filename in
DownloadTargetDeterminer::DoGenerateTargetPath(). It can be adjusted or ignored
based on file system limitations. As I understand, DownloadTargetDeterminer
handles this.

[asanka, 2014-06-26 15:33:42]

On 2014/06/26 10:44:24, Nikhil wrote:
> On 2014/06/25 16:51:00, Tom Sepez wrote:
> > Where do you validate that suggested_filename doesn't contain things like
'..'
> > as part of a path?  Where does this flow to in the browser process?
> 
> suggested_filename is populated based on value of download attribute of anchor
> element. Here's the link to Blink changes -
> https://codereview.chromium.org/334243004/
> 
> suggested_filename will be considered when trying to generate filename in
> DownloadTargetDeterminer::DoGenerateTargetPath(). It can be adjusted or
ignored
> based on file system limitations. As I understand, DownloadTargetDeterminer
> handles this.

suggested_name is passed through net::GenerateFileName() which in turn sanitizes
the string via net::SanitizeGeneratedFileName(). The latter replaces path
separators and leading/trailing '.'s with dashes ('-'). Additional sanitization
steps replace illegal filename characters and name components (e.g. disallow
special names like COM1 or shell folder mounts like Foo.{<GUID>}). Basically the
suggested_name goes through the same sanitization process as names we receive
from Content-Disposition headers or names we derive from the URL.

[Tom Sepez, 2014-06-26 16:22:47]

>suggested_name is passed through net::GenerateFileName() which in turn
sanitizes
> the string via net::SanitizeGeneratedFileName(). The latter replaces path
> separators and leading/trailing '.'s with dashes ('-'). Additional
sanitization
> steps replace illegal filename characters and name components (e.g. disallow
> special names like COM1 or shell folder mounts like Foo.{<GUID>}). Basically
the
> suggested_name goes through the same sanitization process as names we receive
> from Content-Disposition headers or names we derive from the URL.

Great. LGTM.

[Nikhil, 2014-06-26 18:15:02]

The CQ bit was checked by n.bansal@samsung.com

[commit-bot: I haz the power, 2014-06-26 18:17:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/n.bansal@samsung.com/339153002/80001

[lazyboy, 2014-06-26 18:19:39]

On 2014/06/26 18:17:17, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-status.appspot.com/cq/n.bansal@samsung.com/339153002/80001

FYI, You still need OWNERS review for
content/public/common/context_menu_params.h

[commit-bot: I haz the power, 2014-06-26 21:02:32]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/26372)

[commit-bot: I haz the power, 2014-06-26 21:06:46]

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

[commit-bot: I haz the power, 2014-06-26 21:06:47]

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

[Nikhil, 2014-06-26 21:14:42]

On 2014/06/26 18:19:39, lazyboy wrote:
> On 2014/06/26 18:17:17, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-status.appspot.com/cq/n.bansal@samsung.com/339153002/80001
> 
> FYI, You still need OWNERS review for
> content/public/common/context_menu_params.h

I thought Tom's LGTM covered it, based on OWNERS file at content/public/common.

[Nikhil, 2014-06-27 09:10:33]

On 2014/06/26 18:19:39, lazyboy wrote:
> On 2014/06/26 18:17:17, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-status.appspot.com/cq/n.bansal@samsung.com/339153002/80001
> 
> FYI, You still need OWNERS review for
> content/public/common/context_menu_params.h

Whom should I add for OWNERS review for
content/public/common/context_menu_params.h?

[lazyboy, 2014-06-27 14:16:00]

On 2014/06/27 09:10:33, Nikhil wrote:
> On 2014/06/26 18:19:39, lazyboy wrote:
> > On 2014/06/26 18:17:17, I haz the power (commit-bot) wrote:
> > > CQ is trying da patch. Follow status at
> > > 
https://chromium-status.appspot.com/cq/n.bansal@samsung.com/339153002/80001
> > 
> > FYI, You still need OWNERS review for
> > content/public/common/context_menu_params.h
> 
> Whom should I add for OWNERS review for
> content/public/common/context_menu_params.h?

brettw@ is already in the reviewer list, Brett can you take a look?

[Nikhil, 2014-07-01 15:54:36]

On 2014/06/27 14:16:00, lazyboy wrote:
> brettw@ is already in the reviewer list, Brett can you take a look?

brettw@ : <ping!> OWNERS review for content/public/common/context_menu_params.h
please.

[lazyboy, 2014-07-08 15:55:18]

@jochen, can you do an OWNERS review for:
content/public/common/context_menu_params.h?

[brettw, 2014-07-08 23:53:58]

lgtm

[Nikhil, 2014-07-09 06:11:52]

Thanks for reviewing, going to try to submit this now.

[Nikhil, 2014-07-09 06:12:00]

The CQ bit was checked by n.bansal@samsung.com

[commit-bot: I haz the power, 2014-07-09 06:12:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/n.bansal@samsung.com/339153002/80001

[commit-bot: I haz the power, 2014-07-09 07:26:56]

Change committed as 281983