Issue 854943002: Remove RenderViewObserver::Resized (try 2)

Issue 854943002: Remove RenderViewObserver::Resized (try 2) (Closed)

Created:
5 years, 11 months ago by Evan Stade

Modified:
5 years, 11 months ago

Reviewers:
Avi (use Gerrit), jschuh

CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, jam, nasko+codewatch_chromium.org, browser-components-watch_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, estade+watch_chromium.org, rouslan+autofillwatch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Remove RenderViewObserver::Resized (try 2) The only client was AutofillAgent; reimplement the functionality in a slightly different way. While we're at it, fix a bug where the browser-owned popup is hidden without the renderer's knowledge. BUG=433486, 449325 Committed: https://crrev.com/d0a0c992fc0eb9c81d514a256a5c0eb7928efea6 Cr-Commit-Position: refs/heads/master@{#312346}

Patch Set 1 #

Total comments: 5

Patch Set 2 : test compile #

Created: 5 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+56 lines, -21 lines)			Patch
M	chrome/browser/ui/autofill/chrome_autofill_client.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/ui/autofill/chrome_autofill_client.cc	View		1 chunk	+4 lines, -0 lines	0 comments	Download
M	components/autofill/content/browser/content_autofill_driver.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	components/autofill/content/browser/content_autofill_driver.cc	View		1 chunk	+7 lines, -0 lines	0 comments	Download
M	components/autofill/content/common/autofill_messages.h	View		1 chunk	+3 lines, -0 lines	0 comments	Download
M	components/autofill/content/renderer/autofill_agent.h	View		3 chunks	+3 lines, -4 lines	0 comments	Download
M	components/autofill/content/renderer/autofill_agent.cc	View		5 chunks	+6 lines, -12 lines	0 comments	Download
M	components/autofill/core/browser/autofill_driver.h	View		1 chunk	+3 lines, -0 lines	0 comments	Download
M	components/autofill/core/browser/autofill_external_delegate.cc	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	components/autofill/core/browser/test_autofill_driver.h	View	1	1 chunk	+1 line, -0 lines	0 comments	Download
M	components/autofill/core/browser/test_autofill_driver.cc	View	1	1 chunk	+3 lines, -0 lines	0 comments	Download
M	content/browser/renderer_host/render_widget_host_delegate.h	View		1 chunk	+4 lines, -0 lines	0 comments	Download
M	content/browser/renderer_host/render_widget_host_impl.cc	View		1 chunk	+3 lines, -0 lines	0 comments	Download
M	content/browser/web_contents/web_contents_impl.h	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	content/browser/web_contents/web_contents_impl.cc	View		1 chunk	+11 lines, -0 lines	0 comments	Download
M	content/public/browser/web_contents_observer.h	View		1 chunk	+3 lines, -0 lines	0 comments	Download
M	content/public/renderer/render_view_observer.h	View		1 chunk	+0 lines, -1 line	0 comments	Download
M	content/renderer/render_view_impl.cc	View		1 chunk	+0 lines, -4 lines	0 comments	Download

Messages

Total messages: 13 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Avi (use Gerrit)

I'm pondering putting a notification about resizing in the WCO when the embedder _knows_ that ...

5 years, 11 months ago (2015-01-16 20:01:46 UTC) #3

Evan Stade

On 2015/01/16 20:01:46, Avi wrote: > I'm pondering putting a notification about resizing in the ...

5 years, 11 months ago (2015-01-16 20:29:14 UTC) #5

On 2015/01/16 20:01:46, Avi wrote:
> I'm pondering putting a notification about resizing in the WCO when the
embedder
> _knows_ that they resized it. OTOH, there already is shown/hidden.

Does the embedder know? What if you open embedded dev tools? Without searching
through the code I assume blink just partitions part of the native view for the
dev tools.

Also, if we moved the notification upstream, it seems like that would require
updates to lots more code (different platforms, compositing vs. non, etc.). WC
is the cross-platform representation. Unless there's another cross-platform
object that I don't know about.

+jschuh for ipc messages

https://codereview.chromium.org/854943002/diff/1/components/autofill/content/...
File components/autofill/content/common/autofill_messages.h (right):

https://codereview.chromium.org/854943002/diff/1/components/autofill/content/...
components/autofill/content/common/autofill_messages.h:159:
IPC_MESSAGE_ROUTED0(AutofillMsg_PopupHidden);
On 2015/01/16 20:01:45, Avi wrote:
> Was hidden, or should be hidden?

Was hidden. "Should be hidden" would be "HidePopup".

Not sure if this comment was a request to change the message name, but if so,
it's an established pattern that past participle indicates notification of an
action that already happened. See AccountCreationFormsDetected,
FirstUserGestureObserved, FormSeen, etc.

https://codereview.chromium.org/854943002/diff/1/content/public/browser/web_c...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/854943002/diff/1/content/public/browser/web_c...
content/public/browser/web_contents_observer.h:257: // Invoked when the main
frame changes size.
On 2015/01/16 20:01:45, Avi wrote:
> "This method is invoked..." to match the other comments.

or "This methods is invoked" to match the one below...

There are lots of comments that start with "Invoked when" in this file. There
are also lots that start with "This method is invoked when". I think the latter
is needlessly wordy. Since both are established patterns I prefer the more
concise one.

Avi (use Gerrit)

Yeah, let's do it this way. LGTM https://codereview.chromium.org/854943002/diff/1/components/autofill/content/common/autofill_messages.h File components/autofill/content/common/autofill_messages.h (right): https://codereview.chromium.org/854943002/diff/1/components/autofill/content/common/autofill_messages.h#newcode159 components/autofill/content/common/autofill_messages.h:159: IPC_MESSAGE_ROUTED0(AutofillMsg_PopupHidden); I ...

5 years, 11 months ago (2015-01-16 20:35:12 UTC) #6

Evan Stade

Thanks for the review and for chatting yesterday. This solution is better for sure than ...

5 years, 11 months ago (2015-01-16 20:38:27 UTC) #7

Evan Stade

ping jschuh for components/autofill/content/common/autofill_messages.h

5 years, 11 months ago (2015-01-20 22:37:25 UTC) #8

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/854943002/20001

5 years, 11 months ago (2015-01-21 00:39:02 UTC) #11

commit-bot: I haz the power

5 years, 11 months ago (2015-01-21 14:15:02 UTC) #13

Message was sent while issue was closed.

Patchset 2 (id:??) landed as
https://crrev.com/d0a0c992fc0eb9c81d514a256a5c0eb7928efea6
Cr-Commit-Position: refs/heads/master@{#312346}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages