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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 291483010: <webview>: Move name attribute to chrome (Closed)

Created:
6 years, 7 months ago by Fady Samuel
Modified:
6 years, 7 months ago
Reviewers:
lazyboy, jam, nasko
CC:
chromium-reviews, extensions-reviews_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, miu+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@newwindow_refactor
Visibility:
Public.

Description

<webview>: Move name attribute to chrome This CL introduces a new way to monitor changes to frame names from the content embedder, and setting the main frame name from the content embedder. The name attribute is now managed by web_view.js, and not BrowserPlugin. BUG=364141, 330264 TBR=kenrb@chromium.org (removing IPC messages). Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272241

Patch Set 1 #

Patch Set 2 : Fix some tests and cleanup. More cleanup to go. #

Patch Set 3 : All tests pass #

Patch Set 4 : Merge with ToT #

Total comments: 6

Patch Set 5 : Addressed nits #

Patch Set 6 : Addressed John's comment #

Patch Set 7 : Cleanup #

Total comments: 4

Patch Set 8 : Addressed John's comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+199 lines, -145 lines) Patch
M chrome/browser/extensions/api/webview/webview_api.h View 1 chunk +15 lines, -0 lines 0 comments Download
M chrome/browser/extensions/api/webview/webview_api.cc View 1 chunk +15 lines, -0 lines 0 comments Download
M chrome/browser/guest_view/web_view/web_view_constants.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/guest_view/web_view/web_view_constants.cc View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/guest_view/web_view/web_view_guest.h View 1 2 3 4 5 4 chunks +14 lines, -0 lines 0 comments Download
M chrome/browser/guest_view/web_view/web_view_guest.cc View 1 2 3 4 5 6 7 9 chunks +54 lines, -0 lines 0 comments Download
M chrome/common/extensions/api/webview.json View 1 2 3 4 5 6 7 1 chunk +14 lines, -0 lines 0 comments Download
M chrome/common/render_messages.h View 1 2 3 4 5 6 7 2 chunks +9 lines, -0 lines 0 comments Download
M chrome/renderer/chrome_render_frame_observer.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M chrome/renderer/chrome_render_frame_observer.cc View 1 2 3 4 5 4 chunks +12 lines, -1 line 0 comments Download
M chrome/renderer/chrome_render_view_observer.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M chrome/renderer/chrome_render_view_observer.cc View 1 2 3 4 5 6 7 2 chunks +8 lines, -0 lines 0 comments Download
M chrome/renderer/resources/extensions/web_view.js View 1 2 3 4 5 6 chunks +50 lines, -5 lines 0 comments Download
M content/browser/browser_plugin/browser_plugin_guest.h View 1 2 3 4 5 6 2 chunks +0 lines, -6 lines 0 comments Download
M content/browser/browser_plugin/browser_plugin_guest.cc View 1 2 3 4 5 6 10 chunks +0 lines, -36 lines 0 comments Download
M content/common/browser_plugin/browser_plugin_messages.h View 1 4 chunks +0 lines, -12 lines 0 comments Download
M content/common/view_messages.h View 1 2 3 4 5 2 chunks +0 lines, -11 lines 0 comments Download
M content/public/renderer/render_frame_observer.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M content/renderer/browser_plugin/browser_plugin.h View 1 2 2 chunks +0 lines, -5 lines 0 comments Download
M content/renderer/browser_plugin/browser_plugin.cc View 1 7 chunks +0 lines, -24 lines 0 comments Download
M content/renderer/browser_plugin/browser_plugin_bindings.cc View 1 2 chunks +0 lines, -31 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 1 2 3 4 5 1 chunk +1 line, -5 lines 0 comments Download
M content/renderer/render_view_impl.h View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M content/renderer/render_view_impl.cc View 1 2 3 4 5 2 chunks +0 lines, -8 lines 0 comments Download
M extensions/browser/extension_function_histogram_value.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 16 (0 generated)
fsamuel
This isn't done yet but here's my work so far on plumbing the name attribute ...
6 years, 7 months ago (2014-05-16 22:32:36 UTC) #1
Fady Samuel
6 years, 7 months ago (2014-05-20 19:29:00 UTC) #2
Fady Samuel
6 years, 7 months ago (2014-05-20 19:29:12 UTC) #3
Fady Samuel
Hi Istiaque, Could you please take a look at this CL? Thanks, Fady
6 years, 7 months ago (2014-05-21 14:18:07 UTC) #4
lazyboy
Browser plugin code LGTM. https://chromiumcodereview.appspot.com/291483010/diff/50001/chrome/browser/guest_view/web_view/web_view_guest.cc File chrome/browser/guest_view/web_view/web_view_guest.cc (right): https://chromiumcodereview.appspot.com/291483010/diff/50001/chrome/browser/guest_view/web_view/web_view_guest.cc#newcode315 chrome/browser/guest_view/web_view/web_view_guest.cc:315: if (name_.empty()) I think we ...
6 years, 7 months ago (2014-05-21 16:27:05 UTC) #5
Fady Samuel
Nasko, could you please take a look? https://codereview.chromium.org/291483010/diff/50001/chrome/browser/guest_view/web_view/web_view_guest.cc File chrome/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/291483010/diff/50001/chrome/browser/guest_view/web_view/web_view_guest.cc#newcode315 chrome/browser/guest_view/web_view/web_view_guest.cc:315: if (name_.empty()) ...
6 years, 7 months ago (2014-05-21 16:56:48 UTC) #6
Fady Samuel
Nasko, could you please take a look? https://codereview.chromium.org/291483010/diff/50001/chrome/browser/guest_view/web_view/web_view_guest.cc File chrome/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/291483010/diff/50001/chrome/browser/guest_view/web_view/web_view_guest.cc#newcode315 chrome/browser/guest_view/web_view/web_view_guest.cc:315: if (name_.empty()) ...
6 years, 7 months ago (2014-05-21 16:56:48 UTC) #7
Fady Samuel
-nasko@ (who is busy) +jam@
6 years, 7 months ago (2014-05-21 19:09:31 UTC) #8
jam
in general, we don't add stuff to content api which could be done directly by ...
6 years, 7 months ago (2014-05-21 19:20:20 UTC) #9
Fady Samuel
PTAL John
6 years, 7 months ago (2014-05-21 22:20:55 UTC) #10
jam
lgtm with comments https://codereview.chromium.org/291483010/diff/100001/chrome/browser/guest_view/web_view/web_view_guest.cc File chrome/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/291483010/diff/100001/chrome/browser/guest_view/web_view/web_view_guest.cc#newcode1258 chrome/browser/guest_view/web_view/web_view_guest.cc:1258: Send(new ChromeViewMsg_SetName(guest_web_contents()->GetRoutingID(), name_)); s/guest_web_contents()->GetRoutingID()/routing_id() https://codereview.chromium.org/291483010/diff/100001/extensions/browser/extension_function_histogram_value.h File ...
6 years, 7 months ago (2014-05-21 22:49:53 UTC) #11
Fady Samuel
Addressed nits. CQ'ing. https://codereview.chromium.org/291483010/diff/100001/chrome/browser/guest_view/web_view/web_view_guest.cc File chrome/browser/guest_view/web_view/web_view_guest.cc (right): https://codereview.chromium.org/291483010/diff/100001/chrome/browser/guest_view/web_view/web_view_guest.cc#newcode1258 chrome/browser/guest_view/web_view/web_view_guest.cc:1258: Send(new ChromeViewMsg_SetName(guest_web_contents()->GetRoutingID(), name_)); On 2014/05/21 22:49:53, ...
6 years, 7 months ago (2014-05-22 13:36:04 UTC) #12
Fady Samuel
The CQ bit was checked by fsamuel@chromium.org
6 years, 7 months ago (2014-05-22 13:36:09 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fsamuel@chromium.org/291483010/120001
6 years, 7 months ago (2014-05-22 13:37:07 UTC) #14
commit-bot: I haz the power
Change committed as 272241
6 years, 7 months ago (2014-05-22 18:17:50 UTC) #15
nasko
6 years, 7 months ago (2014-05-22 18:27:03 UTC) #16
Message was sent while issue was closed. 
I think some of this code will belong in content, as the frame name will have to
be synchronized across processes, but it can be done at a later time.

Powered by Google App Engine
This is Rietveld 408576698