Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(40)

Issue 2862583010: [Reland 1] Save pdf file instead of web page through context menu for embedded pdf file (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 3 weeks ago by Wei Li (ooo until July 2nd)
Modified:
1 month ago
Reviewers:
lazyboy
CC:
chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove the flaky test as the flaky test is not really necessary. >Save pdf file instead of web page through context menu for embedded pdf file >When user clicks 'Save as...' in the context menu over an embedded pdf file, >we should save the pdf file instead of the entire web page. We fix this by showing >the correct context menu for embedded pdf versus full page pdf: only full page >pdf's context menu shows page navigation items such as 'Reload', >but embedded pdf's context menu doesn't show them. >BUG=chromium:591071 >Review-Url: https://codereview.chromium.org/2862583010 >Cr-Commit-Position: refs/heads/master@{#473105} >Committed: >https://chromium.googlesource.com/chromium/src/+/fd0e534843ac5926c1b26c7e1e7a7229fcb3aeee Review-Url: https://codereview.chromium.org/2862583010 Cr-Commit-Position: refs/heads/master@{#473625} Committed: https://chromium.googlesource.com/chromium/src/+/85a3174dfc2310377872a88db75f7370fa2f9734

Patch Set 1 #

Patch Set 2 : fix file path #

Total comments: 9

Patch Set 3 : rebase #

Patch Set 4 : address comments #

Patch Set 5 : rebase #

Patch Set 6 : fix flaky tests #

Total comments: 8

Patch Set 7 : address more comments #

Patch Set 8 : rebase #

Patch Set 9 : rebase #

Patch Set 10 : battle flakiness #

Patch Set 11 : remove flaky test #

Messages

Total messages: 59 (44 generated)
Wei Li (ooo until July 2nd)
ptal, thanks
1 month, 2 weeks ago (2017-05-06 18:42:06 UTC) #11
Wei Li (ooo until July 2nd)
ping?
1 month, 2 weeks ago (2017-05-11 03:41:39 UTC) #12
lazyboy
Thanks for fixing, some comments below. As I mentioned offline, sorry for not responding earlier. ...
1 month, 2 weeks ago (2017-05-11 18:15:17 UTC) #13
Wei Li (ooo until July 2nd)
ptal, thanks https://codereview.chromium.org/2862583010/diff/20001/chrome/browser/renderer_context_menu/render_view_context_menu.cc File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/2862583010/diff/20001/chrome/browser/renderer_context_menu/render_view_context_menu.cc#newcode1175 chrome/browser/renderer_context_menu/render_view_context_menu.cc:1175: !embedder_web_contents_->IsSavable())) { On 2017/05/11 18:15:17, lazyboy wrote: ...
1 month, 1 week ago (2017-05-17 05:37:10 UTC) #24
lazyboy
https://codereview.chromium.org/2862583010/diff/20001/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/2862583010/diff/20001/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc#newcode721 chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:721: WebContents* inner_contents = web_contents_added_observer.GetWebContents(); On 2017/05/17 05:37:09, Wei Li ...
1 month, 1 week ago (2017-05-17 16:34:45 UTC) #25
Wei Li (ooo until July 2nd)
ptal, thanks https://codereview.chromium.org/2862583010/diff/120001/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/2862583010/diff/120001/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc#newcode170 chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc:170: "setTimeout(\"var l = document.getElementById('link1');" On 2017/05/17 16:34:44, ...
1 month, 1 week ago (2017-05-18 23:25:28 UTC) #34
lazyboy
lgtm, thanks.
1 month, 1 week ago (2017-05-18 23:38:56 UTC) #35
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862583010/160001
1 month, 1 week ago (2017-05-19 03:45:42 UTC) #37
commit-bot: I haz the power
Committed patchset #8 (id:160001) as https://chromium.googlesource.com/chromium/src/+/fd0e534843ac5926c1b26c7e1e7a7229fcb3aeee
1 month, 1 week ago (2017-05-19 06:04:27 UTC) #40
Marc Treib
A revert of this CL (patchset #8 id:160001) has been created in https://codereview.chromium.org/2891923005/ by treib@chromium.org. ...
1 month, 1 week ago (2017-05-19 12:58:08 UTC) #41
Wei Li (ooo until July 2nd)
Patch 10 is the new change. ptal, thanks!
1 month, 1 week ago (2017-05-19 21:50:48 UTC) #48
Wei Li (ooo until July 2nd)
On a second thought, I think it is easier and safer to just remove the ...
1 month ago (2017-05-22 16:01:42 UTC) #53
lazyboy
On 2017/05/22 16:01:42, Wei Li wrote: > On a second thought, I think it is ...
1 month ago (2017-05-22 17:25:10 UTC) #54
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2862583010/220001
1 month ago (2017-05-22 18:07:44 UTC) #56
commit-bot: I haz the power
1 month ago (2017-05-22 18:12:49 UTC) #59
Message was sent while issue was closed.
Committed patchset #11 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/85a3174dfc2310377872a88db75f...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318