Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(385)

Issue 339153002: Use suggested filename for "Save Link As" (Closed)

Created:
6 years, 6 months ago by Nikhil
Modified:
6 years, 5 months ago
CC:
chromium-reviews, darin-cc_chromium.org, jam
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Updated IPC struct #

Patch Set 3 : Added test case #

Total comments: 4

Patch Set 4 : Modified test case #

Total comments: 4

Patch Set 5 : Review feedback #

Unified diffs Side-by-side diffs Delta from patch set Stats (+125 lines, -1 line) Patch
M chrome/browser/renderer_context_menu/render_view_context_menu.cc View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc View 1 2 3 4 1 chunk +33 lines, -0 lines 0 comments Download
M chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.h View 1 2 3 4 2 chunks +31 lines, -0 lines 0 comments Download
M chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc View 1 2 3 4 3 chunks +54 lines, -1 line 0 comments Download
M content/common/frame_messages.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M content/public/common/context_menu_params.h View 1 chunk +4 lines, -0 lines 0 comments Download
M content/renderer/context_menu_params_builder.cc View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 39 (0 generated)
Nikhil
Chrome side changes to use suggested filename when saving the file. PTAL. Thanks!
6 years, 6 months ago (2014-06-17 09:36:50 UTC) #1
kbalazs
On 2014/06/17 09:36:50, Nikhil wrote: > Chrome side changes to use suggested filename when saving ...
6 years, 6 months ago (2014-06-18 21:38:32 UTC) #2
Nikhil
On 2014/06/18 21:38:32, kbalazs wrote: > On 2014/06/17 09:36:50, Nikhil wrote: > > Chrome side ...
6 years, 6 months ago (2014-06-19 05:05:12 UTC) #3
Nikhil
Adding more reviewers based on OWNERS file. PTAL. Also, how do I write test for ...
6 years, 6 months ago (2014-06-19 15:08:59 UTC) #4
lazyboy
On 2014/06/19 15:08:59, Nikhil wrote: > Adding more reviewers based on OWNERS file. PTAL. > ...
6 years, 6 months ago (2014-06-19 18:09:51 UTC) #5
Nikhil
Added test case.
6 years, 6 months ago (2014-06-20 13:34:52 UTC) #6
Nikhil
On 2014/06/19 18:09:51, lazyboy wrote: > On 2014/06/19 15:08:59, Nikhil wrote: > > Adding more ...
6 years, 6 months ago (2014-06-20 13:45:30 UTC) #7
lazyboy
https://chromiumcodereview.appspot.com/339153002/diff/40001/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc (right): https://chromiumcodereview.appspot.com/339153002/diff/40001/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc#newcode235 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 ...
6 years, 6 months ago (2014-06-20 17:23:31 UTC) #8
Nikhil
https://codereview.chromium.org/339153002/diff/40001/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc (right): https://codereview.chromium.org/339153002/diff/40001/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc#newcode235 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 ...
6 years, 6 months ago (2014-06-20 19:37:52 UTC) #9
lazyboy
https://codereview.chromium.org/339153002/diff/40001/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc (right): https://codereview.chromium.org/339153002/diff/40001/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc#newcode235 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 ...
6 years, 6 months ago (2014-06-20 19:50:57 UTC) #10
Nikhil
PTAL. Thanks! https://codereview.chromium.org/339153002/diff/40001/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc (right): https://codereview.chromium.org/339153002/diff/40001/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc#newcode235 chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:235: ContextMenuNotificationObserver menu_observer(IDC_CONTENT_CONTEXT_SAVELINKAS); Modified existing ContextMenuNotificationObserver to consider ...
6 years, 6 months ago (2014-06-23 08:02:35 UTC) #11
lazyboy
Couple of comments about the test util. https://codereview.chromium.org/339153002/diff/60001/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc (right): https://codereview.chromium.org/339153002/diff/60001/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc#newcode33 chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc:33: context_menu_ = ...
6 years, 6 months ago (2014-06-24 01:15:04 UTC) #12
Nikhil
PTAL. I introduced SaveLinkAsContextMenuObserver, and removed the changes from ContextMenuNotificationObserver. Thanks!
6 years, 6 months ago (2014-06-24 10:38:43 UTC) #13
Nikhil
https://codereview.chromium.org/339153002/diff/60001/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc File chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc (right): https://codereview.chromium.org/339153002/diff/60001/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc#newcode33 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: > ...
6 years, 6 months ago (2014-06-24 10:39:52 UTC) #14
lazyboy
render_view_context_menu* LGTM. Thanks for making the changes.
6 years, 6 months ago (2014-06-24 14:25:51 UTC) #15
Nikhil
On 2014/06/24 14:25:51, lazyboy wrote: > render_view_context_menu* LGTM. > Thanks for making the changes. Great! ...
6 years, 6 months ago (2014-06-24 15:05:29 UTC) #16
lazyboy
On 2014/06/24 15:05:29, Nikhil wrote: > On 2014/06/24 14:25:51, lazyboy wrote: > > render_view_context_menu* LGTM. ...
6 years, 6 months ago (2014-06-24 15:29:15 UTC) #17
Nikhil
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: ...
6 years, 6 months ago (2014-06-25 06:20:56 UTC) #18
Tom Sepez
Where do you validate that suggested_filename doesn't contain things like '..' as part of a ...
6 years, 6 months ago (2014-06-25 16:51:00 UTC) #19
jamesr
the /renderer/ side lgtm, but you definitely need to validate this carefully on the browser ...
6 years, 6 months ago (2014-06-25 18:16:16 UTC) #20
Nikhil
On 2014/06/25 16:51:00, Tom Sepez wrote: > Where do you validate that suggested_filename doesn't contain ...
6 years, 6 months ago (2014-06-26 10:44:24 UTC) #21
asanka
On 2014/06/26 10:44:24, Nikhil wrote: > On 2014/06/25 16:51:00, Tom Sepez wrote: > > Where ...
6 years, 6 months ago (2014-06-26 15:33:42 UTC) #22
Tom Sepez
>suggested_name is passed through net::GenerateFileName() which in turn sanitizes > the string via net::SanitizeGeneratedFileName(). The ...
6 years, 6 months ago (2014-06-26 16:22:47 UTC) #23
Nikhil
The CQ bit was checked by n.bansal@samsung.com
6 years, 6 months ago (2014-06-26 18:15:02 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/n.bansal@samsung.com/339153002/80001
6 years, 6 months ago (2014-06-26 18:17:17 UTC) #25
lazyboy
On 2014/06/26 18:17:17, I haz the power (commit-bot) wrote: > CQ is trying da patch. ...
6 years, 6 months ago (2014-06-26 18:19:39 UTC) #26
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium ...
6 years, 6 months ago (2014-06-26 21:02:32 UTC) #27
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 6 months ago (2014-06-26 21:06:46 UTC) #28
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/builds/76675)
6 years, 6 months ago (2014-06-26 21:06:47 UTC) #29
Nikhil
On 2014/06/26 18:19:39, lazyboy wrote: > On 2014/06/26 18:17:17, I haz the power (commit-bot) wrote: ...
6 years, 6 months ago (2014-06-26 21:14:42 UTC) #30
Nikhil
On 2014/06/26 18:19:39, lazyboy wrote: > On 2014/06/26 18:17:17, I haz the power (commit-bot) wrote: ...
6 years, 6 months ago (2014-06-27 09:10:33 UTC) #31
lazyboy
On 2014/06/27 09:10:33, Nikhil wrote: > On 2014/06/26 18:19:39, lazyboy wrote: > > On 2014/06/26 ...
6 years, 5 months ago (2014-06-27 14:16:00 UTC) #32
Nikhil
On 2014/06/27 14:16:00, lazyboy wrote: > brettw@ is already in the reviewer list, Brett can ...
6 years, 5 months ago (2014-07-01 15:54:36 UTC) #33
lazyboy
@jochen, can you do an OWNERS review for: content/public/common/context_menu_params.h?
6 years, 5 months ago (2014-07-08 15:55:18 UTC) #34
brettw
lgtm
6 years, 5 months ago (2014-07-08 23:53:58 UTC) #35
Nikhil
Thanks for reviewing, going to try to submit this now.
6 years, 5 months ago (2014-07-09 06:11:52 UTC) #36
Nikhil
The CQ bit was checked by n.bansal@samsung.com
6 years, 5 months ago (2014-07-09 06:12:00 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/n.bansal@samsung.com/339153002/80001
6 years, 5 months ago (2014-07-09 06:12:42 UTC) #38
commit-bot: I haz the power
6 years, 5 months ago (2014-07-09 07:26:56 UTC) #39
Message was sent while issue was closed.
Change committed as 281983

Powered by Google App Engine
This is Rietveld 408576698