Add zoom extension API

Add zoom extension API

BUG=30583

[andersrb, 2010-02-09 00:20:32]

This patch adds a simple zoom in/out extension API. Bug 30583.

[jv, 2010-05-20 20:00:55]

can this be made part of Chome 6?

On 2010/02/09 00:20:32, andersrb wrote:
> This patch adds a simple zoom in/out extension API. Bug 30583.

[andersrb, 2010-10-18 03:56:07]

Patch 5 gives extension_tabs_module.cc a dependency on webkit by including
WebView.h and using the zoomFactorToZoomLevel and zoomLevelToZoomFactor
functions. I don't know if that's correct.

[Aaron Boodman, 2010-10-18 20:16:43]

SetZoomLevel() calls the RenderView directly, while GetZoomLevel() talks to
host_zoom_map.cc. If I'm reading the code right, it looks like this will result
in the below not working.

chrome.tabs.setZoomLevel(tabId, zoomLevel, function() {
  chrome.tabs.getZoomLevel(tabId, function(newLevel) {
    assertEq(zoomLevel, newLevel);
  });
});

Am I missing something?

Would it work to just make SetZoomLevel delegate to
HostZoomMap::SetTemporaryZoomLevel()?

[andersrb, 2010-10-19 02:39:28]

chrome.tabs.setZoomPercent(tabId, zoomPercent);
  chrome.tabs.getZoomPercent(tabId, function(newZoomPercent) {
    alert(newZoomPercent);
  });

[andersrb, 2010-10-19 02:44:31]

Please ignore the previous message. It was a misfire.

The example as you proposed it doesn't work because setZoomPercent doesn't
accept a callback, but the following code is racy:

chrome.tabs.setZoomPercent(tabId, zoomPercent);
chrome.tabs.getZoomPercent(tabId, function(newZoomPercent) {
  alert(newZoomPercent);
});

Sometimes it gets the new zoom level, sometimes the old. I guess that's because
of the async IPC? I'll investigate your suggestion.

Another issue I noticed: calling setZoomPercent from a page action seems to
cause the page action to be reloaded. Do you think that's as expected? It seems
wierd.

[andersrb, 2010-10-31 22:46:53]

This version of the patch attempts to solve the race condition in the previous
patch wherein calling getZoomPercent after setZoomPercent could return the wrong
value.

To do this I used HostZoomMap's temporary zoom features, but after doing so it
was obvious that temporary zoom really wanted to live in TabControl, so I moved
it.

I also removed the 'remember' flag from ZoomLimits update message in favor of
having RenderView always notify the UI process of the zoom level whenever the
URL changes.

With this API, creating zoom in and zoom out functions in an extension that
mimic chrome's took me around 90 verbose lines of javascript and required
duplicating code from WebViewImpl.cpp and render_view.cc:

http://github.com/brson/zoomapitest/blob/master/popup.html

I've also created another smoothgestures fork that uses this API (zoomapi
branch):

git@github.com:brson/smoothgestures.git

With this patch I've seen my 32-bit, nswrapped flash plugin crash when receiving
many zoom requests in quick succession. More testing needed there. I also need
to do more testing with full-page plugns to make sure I didn't break the
existing temporary zoom behavior.

[Aaron Boodman, 2010-11-15 21:56:32]

+pkasting

erikkay, pkasting: can you take a look at this CL in your spare time and
validate whether my concerns are well-founded or not? I don't want to waste more
of Brian's time if his original approach was OK.

Basically, right now WebCore deals with "zoom" in terms of percentage
(WebFrame::setPageAndZoomFactor). Chromium mostly deals with it in terms of
"level".

Initially Brian created an API to Chrome's current concepts, that is levels. My
concern is that the implementation of zoom in Chromium could change over time,
obsoleting API built around levels. Also, percentage seems more flexible.

However, it's a lot more work to do an API based on percentages, and it also
makes things harder for any developer that wants to emulate Chrome's current
zoom behavior (but I don't really buy that use case too much).

Thoughts?

[Peter Kasting, 2010-11-15 23:28:18]

While I did some of the original implementation, I don't have a good feel for
how zoom will work in the future.

What's been driving zoom looking forward is the PDF integration, which affects
the UI and capabilities we want to have.  My wild guess is that a
percentage-based system is indeed what we want.  But the PDF folks would know
more.  I'm not sure who the best person is.  I'll find someone and ask them to
comment here.

[Erik does not do reviews, 2010-11-16 00:09:45]

From reading the bug, it sounds like andersb just wants to emulate current
behavior but with gestures.  The easiest way to support that would be to have a
zoomIn/zoomOut/zoomReset set of functions.  If we did that, we wouldn't need to
expose our internal representations.  Maybe this is an easier place to start?

I don't know enough about zoom and its future to have a strong opinion about
whether using percentage is better in the long run.  It certainly seems easier
to understand conceptually, but on the other hand since the current behavior is
non-linear, you could imagine us tweaking those levels over time to make them
nicer (maybe different for different devices and form factors even).  By
exposing the raw percent instead of the level, it might make it harder for
extensions to capture the behavior that the browser has.

[Peter Kasting, 2010-11-16 00:36:21]

John, can you comment here regarding Aaron's question above?

[jv, 2010-11-16 01:02:52]

On 2010/11/16 00:36:21, Peter Kasting wrote:
> John, can you comment here regarding Aaron's question above?
Not sure how I got on this, I starred an issue to follow it about a year ago. 
Can I be removed from this.  I don't have the knowledge to contribute.

[jam, 2010-11-17 00:59:11]

I agree that percentages is better for extensions.  This is what we had done for
pepper plugins (search for 'zoom' in
src\webkit\glue\plugins\pepper_plugin_instance.cc).

Please test this thoroughly with the PDF plugin.  You'll want to ensure that
when the fit-to-width/fit-to-height buttons are pressed, and then the user
starts zooming using the +/- buttons or the menu, that the user can get to 100%
exactly (i.e. no rounding errors).

[andersrb, 2010-11-21 20:48:13]

Thanks for the feedback, Gentlemen. John, those are great suggestions and I will
explore them.

I'll also point out that downchuck has brought up some good considerations on
the issue ticket discussion. He thinks this belongs in the DOM and links to a MS
proposal that seems to cover this area, which would require the solution to go
into WebKit.

[Aaron Boodman, 2010-11-22 00:42:53]

I'm having trouble understanding why the current code has this weird round trip
in it where the zoom change starts in the browser, then goes to the renderer,
then bounces back to the browser. It would be simpler if we just made the change
in the browser process, then told the renderers about it. But there must be some
reason this isn't the way it works, right?

[Aaron Boodman, 2010-11-22 00:43:22]

(other than that, I think this is on the right track -- thanks for sticking with
it)

[andersrb, 2010-11-22 01:28:18]

It's because the render process is responsible for deciding what the final zoom
level is. It can decide to adjust the zoom level (e.g. when you hit the limits)
or decide to not do anything (e.g. for plugins).

So really, that ugly trick of storing the temporary zoom level until the
renderer replies may actually end up storing the wrong zoom level briefly.

One solution that might make zoom handling much simpler is to perform all the
logic in the UI process, as you suggest. The only tricky bit there is that we
would still need to handle changes that originate in the render process (maybe
just for plugins - not sure).

[andersrb, 2010-11-22 01:28:18]

It's because the render process is responsible for deciding what the final zoom
level is. It can decide to adjust the zoom level (e.g. when you hit the limits)
or decide to not do anything (e.g. for plugins).

So really, that ugly trick of storing the temporary zoom level until the
renderer replies may actually end up storing the wrong zoom level briefly.

One solution that might make zoom handling much simpler is to perform all the
logic in the UI process, as you suggest. The only tricky bit there is that we
would still need to handle changes that originate in the render process (maybe
just for plugins - not sure).

[Peter Kasting, 2010-11-22 02:05:41]

On 2010/11/22 01:28:18, andersrb wrote:
> It's because the render process is responsible for deciding what the final
zoom
> level is. It can decide to adjust the zoom level (e.g. when you hit the
limits)
> or decide to not do anything (e.g. for plugins).

That's part of it, but not all.

The real reason we did things this way is because the renderer and the browser
each have their own concept of what page the user is on, and those concepts can
be out of sync during a page transition (e.g. because the renderer has switched
to a new page but the browser hasn't gotten the message to do that yet).

Consider the case of the renderer and browser being on page A.  Now the renderer
navigates to page B.  As the messages associated with that are in flight, the
user zooms.  If the browser does everything itself, it will zoom page A.  Now it
sends the message to the renderer.  Two problems are possible:
(a) The browser tells the renderer "zoom what you're on" and so the renderer
zooms page B.  Now the renderer and browser are out of sync.
(b) We avoid problem (a) by specifically saying "zoom page A".  Now the renderer
does nothing and Chrome looks unresponsive.

When you couple these problems with the ones already mentioned regarding the
renderer making further decisions about how to zoom, it becomes easier to let
the renderer do all the work and inform the browser of the result.

Then there is one final step -- the browser rebroadcasts the new zoom level for
page B so that any other renderers showing it will also update.

[Aaron Boodman, 2010-11-22 02:11:30]

Ok, I think I understand all the issues now.

How about this... since what is really going on here is asynchronous, let's
embrace it instead of trying to fight it.

We can make setZoom have a callback that gets called once the zoom has taken
effect. That way there's no need to store the temporary value, and we can keep
the majority of the code the way it was before (reducing the scariness of the
change).

In order to support a callback in setZoom(), you will need to:

- Make your TabContents::SetZoomPercent() take an optional request_id argument
that will be sent back when the zoom is completed.

- Make SetZoomTabFunction inherit from AsyncExtensionFunction instead of
SyncExtensionFunction.

- Add request_id to the ZOOM_LEVEL_CHANGED notification. Pass the request_id
through from the tab contents, to the renderer, and then send it back through to
ZoomHostMap so that it can send it with the notification.

- Make SetZoomTabFunction observe ZOOM_LEVEL_CHANGED notifications, and when it
sees the one it's waiting for, fire its callback.

I think this should allow you to get rid of some of the complexity from your
current change, right?

Also, if you would like to get something checked in to make progress, I am OK
with doing the callback in a separate change.

[Aaron Boodman, 2010-11-22 02:16:28]

On Sun, Nov 21, 2010 at 6:11 PM,  <aa@chromium.org> wrote:
> Ok, I think I understand all the issues now.
>
> How about this... since what is really going on here is asynchronous, let's
> embrace it instead of trying to fight it.
>
> We can make setZoom have a callback that gets called once the zoom has taken
> effect. That way there's no need to store the temporary value, and we can
> keep
> the majority of the code the way it was before (reducing the scariness of
> the
> change).
>
> In order to support a callback in setZoom(), you will need to:
>
> - Make your TabContents::SetZoomPercent() take an optional request_id
> argument
> that will be sent back when the zoom is completed.
>
> - Make SetZoomTabFunction inherit from AsyncExtensionFunction instead of
> SyncExtensionFunction.
>
> - Add request_id to the ZOOM_LEVEL_CHANGED notification. Pass the request_id
> through from the tab contents, to the renderer, and then send it back
> through to
> ZoomHostMap so that it can send it with the notification.
>
> - Make SetZoomTabFunction observe ZOOM_LEVEL_CHANGED notifications, and when
> it
> sees the one it's waiting for, fire its callback.
>
> I think this should allow you to get rid of some of the complexity from your
> current change, right?
>
> Also, if you would like to get something checked in to make progress, I am
> OK
> with doing the callback in a separate change.

You can take a look at some of the other AsyncExtensionFunctions'
implementations for inspiration. Many use this same pattern. For
example, the DetectLanguageTabFunction is similar.

- a

[andersrb, 2010-11-22 02:34:57]

Thanks for the clarifications, Peter. Aaron, I like your suggestion, and will
move in that direction. I don't want to check in what I've got now; after all
the feedback today I'm confident that it is not right yet. Please allow me to
have another iteration.

I could also use a hint about how to test libpdf against trunk. So far I've
tried to copy it from a dev channel installation to my build folder, but I've
gotten errors like "Plugin doesn't support instance interface, failing." and
"ASSERTION FAILED: m_frame->document()->parsing()". I'm running Linux. Will keep
experimenting.

[andersrb, 2010-11-29 04:50:09]

Patch 7 implements SetZoomPercent with an asynchronous callback as suggested by
Aaron. This results in a much less invasive, and more obviously correct,
changeset.

This patch includes unit tests.

I still have two concerns. First, SetZoomPercentTabFunction has an open-coded
check for the validity of one of its arguments, not using
EXTENSION_FUNCTION_VALIDATE. This is because the argument can be either real or
integer. I don't see other methods doing this, though extension_tts_api_util.cc
has a ReadNumberByKey method that does something similar.

Second, I don't think that SetZoomPercentTabFunction is correct in the event
that it doesn't receive a response to it's zoom request, which can happen if the
tab content is a plugin. It looks like it will leak a reference to itself, and
not respond to the extension script.

Relatedly, I'm not sure what's supposed to happen if the tab changes or closes
between the request and the response. I see that DetectTabLanguageFunction
registers interest in TAB_CLOSING and NAV_ENTRY_COMMITTED notifications, but
then doesn't appear to do anything with them.

Advice on those issues is needed.

Although I believe this patch is getting close to ready, I have not included the
rebuilt extension documentation as my results looked significantly different
than what's in trunk.

And finally, I still have not been able to test this change with libpdf.so for
the reasons mentioned in my previous post. I think this patch has a smaller
likelyhood of breaking anything there though.

[andersrb, 2011-02-07 01:35:39]

Unless somebody expresses further concerns, I believe this feature is complete
with patch 8. It includes tests, docs and an example. I've tested it with
libpdf.so on Linux.

It has one weird issue: when zooming to a non-integral (non-standard) zoom level
on libpdf WebViewImpl doesn't immediately report the zoom level correctly; it
does report the zoom level correctly if the request is made twice. This looks
like a problem for libpdf though and has a workaround for extension writers
(though I don't know if they can currently distinguish html pages from plugin
pages).

[Aaron Boodman, 2011-02-07 09:29:12]

omg, it's so beautiful.

I only had a few comments, and in order to move things along faster, I went
ahead and added them to your patch, here:

http://codereview.chromium.org/6413014/ (you can see the changes by clicking the
'1' next to each filename).

If you're OK with the changes, let me know and I'll land the change for you.

[andersrb, 2011-02-07 15:14:49]

Yes. Looks good.

On Mon, Feb 7, 2011 at 4:29 AM, <aa@chromium.org> wrote:

> omg, it's so beautiful.
>
> I only had a few comments, and in order to move things along faster, I went
> ahead and added them to your patch, here:
>
> http://codereview.chromium.org/6413014/ (you can see the changes by
> clicking the
> '1' next to each filename).
>
> If you're OK with the changes, let me know and I'll land the change for
> you.
>
>
> http://codereview.chromium.org/570048/
>

[Aaron Boodman, 2011-02-15 02:03:57]

Haven't forgotten about this. Got sidetracked trying to add some features to the
change.

Will land it someday.

[Peter Kasting, 2011-04-21 20:21:07]

On 2011/02/15 02:03:57, Aaron Boodman wrote:
> Will land it someday.

Aaron, what's the status of this patch?  It looks like the CL you mention a few
comments up was closed, but the associated bug is not marked Fixed and shows no
commit-bot additions.