Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
302603012:
Zoom Extension API (content changes) (Closed)
|
Created:
6 years, 6 months ago by wjmaclean Modified:
6 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionZoom Extension API (content changes)
Changes to content/ needed to facilitate the new zoom extension API,
which is outlined in:
https://docs.google.com/a/chromium.org/document/d/1sCZjx1J3_M2a02T8NXd-ufGKZDoBHI5Ixis1DaGLQCA/edit?usp=sharing
Based on code from https://codereview.chromium.org/226523006/, with
various fixes and rebased against changes in
https://codereview.chromium.org/287093002/
Must land before https://codereview.chromium.org/301733006/.
BUG=30583
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277153
Patch Set 1 #
Total comments: 1
Patch Set 2 : In progress, uploaded for testing only. #Patch Set 3 : Additions to HostZoomMap API. #
Total comments: 21
Patch Set 4 : Address comments. #
Total comments: 1
Patch Set 5 : Remove exceptions from IPC. #
Total comments: 38
Patch Set 6 : Address comments. #
Total comments: 14
Patch Set 7 : Use ContainsKey(), send host zoom level if it exists. #
Total comments: 8
Patch Set 8 : Fix nits. #
Total comments: 14
Patch Set 9 : Fix nits. #
Total comments: 6
Patch Set 10 : Added comments re GURL vs host/scheme use. #
Messages
Total messages: 30 (0 generated)
Moved from https://codereview.chromium.org/226523006/
https://codereview.chromium.org/302603012/diff/1/content/renderer/render_thre... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/302603012/diff/1/content/renderer/render_thre... content/renderer/render_thread_impl.cc:1200: const std::set<int>& exceptions) { I'm confused. What are we using exceptions for?
On 2014/05/28 15:37:05, fsamuel wrote: > https://codereview.chromium.org/302603012/diff/1/content/renderer/render_thre... > File content/renderer/render_thread_impl.cc (right): > > https://codereview.chromium.org/302603012/diff/1/content/renderer/render_thre... > content/renderer/render_thread_impl.cc:1200: const std::set<int>& exceptions) { > I'm confused. What are we using exceptions for? Sorry, wasn't expecting this to be reviewed just yet. I don't know yet if this is needed or not. I've only just gotten all Paul's original code to run, and I'm in the process of evaluating against the design doc to see what else remains to be done. At present these don't have an apparent use, but I haven't removed them yet as I'd like to be certain before I do.
On 2014/05/28 15:40:30, wjmaclean wrote: > On 2014/05/28 15:37:05, fsamuel wrote: > > > https://codereview.chromium.org/302603012/diff/1/content/renderer/render_thre... > > File content/renderer/render_thread_impl.cc (right): > > > > > https://codereview.chromium.org/302603012/diff/1/content/renderer/render_thre... > > content/renderer/render_thread_impl.cc:1200: const std::set<int>& exceptions) > { > > I'm confused. What are we using exceptions for? > > Sorry, wasn't expecting this to be reviewed just yet. > > I don't know yet if this is needed or not. I've only just gotten all Paul's > original code to run, and I'm in the process of evaluating against the design > doc to see what else remains to be done. At present these don't have an apparent > use, but I haven't removed them yet as I'd like to be certain before I do. The original use was to isolate manual zoom RenderViews from other RenderViews that don't have manual zoom within the same process. I feel like this might still apply.
On 2014/05/28 15:42:09, fsamuel wrote: > > The original use was to isolate manual zoom RenderViews from other RenderViews > that don't have manual zoom within the same process. I feel like this might > still apply. Then that will be confirmed when we have a testing platform.
This cl, in conjunction with https://codereview.chromium.org/301733006/, seems to be fairly mature now, so I'd like to start getting review feedback.
I have a bunch of API level comments. It might be easier to take this offline. https://codereview.chromium.org/302603012/diff/40001/content/browser/host_zoo... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/302603012/diff/40001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:308: const std::string& host, This is really weird and inconsistent with the expectations of the API. Why do we need to pass in a host. From an API level, "temporary zoom" translates into per-renderer zoom. Why does the host matter? Can we extract the current host from some other place? From the chrome code, it looks like we're extracting the host on the chrome side via: content::NavigationEntry* entry = web_contents()->GetController().GetLastCommittedEntry(); std::string host = net::GetHostOrSpecFromURL(entry ? entry->GetURL() : GURL::EmptyGURL()); This can be done here instead, simplifying the API we need to expose to something that better matches the expectations of a content embedder developer. https://codereview.chromium.org/302603012/diff/40001/content/common/view_mess... File content/common/view_messages.h (right): https://codereview.chromium.org/302603012/diff/40001/content/common/view_mess... content/common/view_messages.h:688: std::set<int> /* exceptions */) It seems kind of heavy weight to iterate through and carry an exception level across processes with every zoom level change. I wonder if it would be more efficient to store an exception bit in the renderer and update that bit through another IPC. https://codereview.chromium.org/302603012/diff/40001/content/common/view_mess... content/common/view_messages.h:691: IPC_MESSAGE_ROUTED1(ViewMsg_SetZoomLevelForView, To the comment above, the first parameter sets the exception bit, and the second sets the zoom level. ViewMsg_SetTemporaryZoomLevel(bool /* enabled */, double /* level */) https://codereview.chromium.org/302603012/diff/40001/content/public/browser/h... File content/public/browser/host_zoom_map.h (right): https://codereview.chromium.org/302603012/diff/40001/content/public/browser/h... content/public/browser/host_zoom_map.h:33: class HostZoomMap { I find this name really confusing, especially with the direction we're going in with zoom. Could we please call this ZoomManager? https://codereview.chromium.org/302603012/diff/40001/content/public/browser/h... content/public/browser/host_zoom_map.h:85: virtual bool HasZoomLevelInMap(const std::string& scheme, I'm not a fan of calling this thing a map at all at the content API level. This class isn't even a map anymore. Please call this function HasZoomLevel. https://codereview.chromium.org/302603012/diff/40001/content/public/browser/h... content/public/browser/host_zoom_map.h:120: bool send_level_to_view) =0; This last parameter is really confusing at an API level. I wouldn't know how to use it without looking at the code in chrome. I feel that we need two methods here: 1. To update the bookkeeping. 2. Set the zoom level according to the bookkeeping. I'm not yet sure what the best naming is here but this https://codereview.chromium.org/302603012/diff/40001/content/public/browser/h... content/public/browser/host_zoom_map.h:125: virtual void EraseTemporaryZoomLevel(int render_process_id, name: ClearTemporaryZoomLevel https://codereview.chromium.org/302603012/diff/40001/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/302603012/diff/40001/content/renderer/render_... content/renderer/render_view_impl.cc:674: next_snapshot_id_(0) {} Can we undo this change? https://codereview.chromium.org/302603012/diff/40001/content/renderer/render_... content/renderer/render_view_impl.cc:2665: webview()->setZoomLevel(level); Do we need to close popups here? https://codereview.chromium.org/302603012/diff/40001/content/renderer/render_... content/renderer/render_view_impl.cc:2666: // zoomLevelChanged(); Remove this?
I'll incorporate the changes into the next uploaded patch. See comments below. https://codereview.chromium.org/302603012/diff/40001/content/browser/host_zoo... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/302603012/diff/40001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:308: const std::string& host, On 2014/06/09 15:43:21, Fady Samuel wrote: > This is really weird and inconsistent with the expectations of the API. Why do > we need to pass in a host. From an API level, "temporary zoom" translates into > per-renderer zoom. Why does the host matter? Can we extract the current host > from some other place? > > From the chrome code, it looks like we're extracting the host on the chrome side > via: > > content::NavigationEntry* entry = > web_contents()->GetController().GetLastCommittedEntry(); > std::string host = > net::GetHostOrSpecFromURL(entry ? entry->GetURL() : GURL::EmptyGURL()); > > This can be done here instead, simplifying the API we need to expose to > something that better matches the expectations of a content embedder developer. > Sure, we can do that. I added it primarily so it could be included in the ZoomLevelChange event, and on the supposition that the caller likely already had the information handy. https://codereview.chromium.org/302603012/diff/40001/content/common/view_mess... File content/common/view_messages.h (right): https://codereview.chromium.org/302603012/diff/40001/content/common/view_mess... content/common/view_messages.h:688: std::set<int> /* exceptions */) On 2014/06/09 15:43:21, Fady Samuel wrote: > It seems kind of heavy weight to iterate through and carry an exception level > across processes with every zoom level change. I wonder if it would be more > efficient to store an exception bit in the renderer and update that bit through > another IPC. Personally, I would think that a single IPC would be more efficient, since the majority of the cost would be in setting up the message, and doing the transmission. That being said, I have never seen performance metrics for this sort of thing, so I can't be sure. In most cases I would expect the set to be small or even empty. The cost of the iteration is immaterial, as iterating over the temporary zoom levels list in HostZoomMap should be small, we'd still have to do the iteration in RenderThreadImpl. If we *did* go this route, then I would suggest staying with the IPC below, but knowing that once this is called on a render view that it automatically puts itself into "temporary" mode, and RenderThreadImpl checks the status of each view before trying to call SetZoomLevel on it. There would need to be another IPC to reset the view though. https://codereview.chromium.org/302603012/diff/40001/content/public/browser/h... File content/public/browser/host_zoom_map.h (right): https://codereview.chromium.org/302603012/diff/40001/content/public/browser/h... content/public/browser/host_zoom_map.h:33: class HostZoomMap { On 2014/06/09 15:43:21, Fady Samuel wrote: > I find this name really confusing, especially with the direction we're going in > with zoom. Could we please call this ZoomManager? I'm OK with that. But, we should then consider the potential for confusion between ZoomManager and ZoomController. Eventually perhaps the two should merge? That might be more than we want to put into this CL though. https://codereview.chromium.org/302603012/diff/40001/content/public/browser/h... content/public/browser/host_zoom_map.h:85: virtual bool HasZoomLevelInMap(const std::string& scheme, On 2014/06/09 15:43:21, Fady Samuel wrote: > I'm not a fan of calling this thing a map at all at the content API level. This > class isn't even a map anymore. Please call this function HasZoomLevel. Done. https://codereview.chromium.org/302603012/diff/40001/content/public/browser/h... content/public/browser/host_zoom_map.h:120: bool send_level_to_view) =0; On 2014/06/09 15:43:21, Fady Samuel wrote: > This last parameter is really confusing at an API level. I wouldn't know how to > use it without looking at the code in chrome. > > I feel that we need two methods here: > > 1. To update the bookkeeping. > 2. Set the zoom level according to the bookkeeping. > > I'm not yet sure what the best naming is here but this Ok, I can look into separating this. One consideration is that this is used because the HostZoomMap doesn't know anything about the zoom modes, manual vs. isolated. If we merge ZoomController and HostZoomMap then it will, but in the short term the sender of messages needs to know who should get one (isolated) and who shouldn't (manual). Perhaps this should be stored in the temporary zoom settings, and we'll rename the parameter here ('bool send_level_to_view' becomes 'enum mode {manual, isolated}'?) https://codereview.chromium.org/302603012/diff/40001/content/public/browser/h... content/public/browser/host_zoom_map.h:125: virtual void EraseTemporaryZoomLevel(int render_process_id, On 2014/06/09 15:43:21, Fady Samuel wrote: > name: ClearTemporaryZoomLevel Done. https://codereview.chromium.org/302603012/diff/40001/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/302603012/diff/40001/content/renderer/render_... content/renderer/render_view_impl.cc:674: next_snapshot_id_(0) {} On 2014/06/09 15:43:21, Fady Samuel wrote: > Can we undo this change? Done. Leftover debugging artifact I suspect. https://codereview.chromium.org/302603012/diff/40001/content/renderer/render_... content/renderer/render_view_impl.cc:2665: webview()->setZoomLevel(level); On 2014/06/09 15:43:21, Fady Samuel wrote: > Do we need to close popups here? Probably ... consider it added (unless I discover a good reason why we shouldn't ... which I vaguely think there might be ...). https://codereview.chromium.org/302603012/diff/40001/content/renderer/render_... content/renderer/render_view_impl.cc:2666: // zoomLevelChanged(); On 2014/06/09 15:43:21, Fady Samuel wrote: > Remove this? Done.
I've uploaded the new patch. 1) I suspect there's more cost in setting up and sending an IPC than there is in sending a small exception set, so I've left this unchanged. Let me know if I'm wrong. 2) I think re-naming HostZoomMap belongs in its own CL (to simplify landing this one, and to make the reasons around the naming clear and independent of the work in this CL). See further comments below. https://codereview.chromium.org/302603012/diff/40001/content/browser/host_zoo... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/302603012/diff/40001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:308: const std::string& host, On 2014/06/09 17:23:29, wjmaclean wrote: > On 2014/06/09 15:43:21, Fady Samuel wrote: > > This is really weird and inconsistent with the expectations of the API. Why do > > we need to pass in a host. From an API level, "temporary zoom" translates into > > per-renderer zoom. Why does the host matter? Can we extract the current host > > from some other place? > > > > From the chrome code, it looks like we're extracting the host on the chrome > side > > via: > > > > content::NavigationEntry* entry = > > web_contents()->GetController().GetLastCommittedEntry(); > > std::string host = > > net::GetHostOrSpecFromURL(entry ? entry->GetURL() : GURL::EmptyGURL()); > > > > This can be done here instead, simplifying the API we need to expose to > > something that better matches the expectations of a content embedder > developer. > > > > Sure, we can do that. > > I added it primarily so it could be included in the ZoomLevelChange event, and > on the supposition that the caller likely already had the information handy. Done. https://codereview.chromium.org/302603012/diff/40001/content/public/browser/h... File content/public/browser/host_zoom_map.h (right): https://codereview.chromium.org/302603012/diff/40001/content/public/browser/h... content/public/browser/host_zoom_map.h:120: bool send_level_to_view) =0; On 2014/06/09 17:23:29, wjmaclean wrote: > On 2014/06/09 15:43:21, Fady Samuel wrote: > > This last parameter is really confusing at an API level. I wouldn't know how > to > > use it without looking at the code in chrome. > > > > I feel that we need two methods here: > > > > 1. To update the bookkeeping. > > 2. Set the zoom level according to the bookkeeping. > > > > I'm not yet sure what the best naming is here but this > > Ok, I can look into separating this. > > One consideration is that this is used because the HostZoomMap doesn't know > anything about the zoom modes, manual vs. isolated. If we merge ZoomController > and HostZoomMap then it will, but in the short term the sender of messages needs > to know who should get one (isolated) and who shouldn't (manual). Perhaps this > should be stored in the temporary zoom settings, and we'll rename the parameter > here ('bool send_level_to_view' becomes 'enum mode {manual, isolated}'?) Done.
https://codereview.chromium.org/302603012/diff/60001/content/common/view_mess... File content/common/view_messages.h (right): https://codereview.chromium.org/302603012/diff/60001/content/common/view_mess... content/common/view_messages.h:689: std::set<int> /* exceptions */) I'm still not particularly comfortable about this exceptions list. A developer coming in to add new zoom functionality would not have an understanding of why there's an exception list. Also, it's not clear how SetZoomLevelForView interacts with SetZoomLevelForCurrentURL. If the exception bit is renderer-side, set form the other IPC (renaming the other IPC to highlight the terminology "TemporaryZoomLevel"), then it becomes clear how these two IPCs mesh together.
On 2014/06/10 14:24:27, Fady Samuel wrote: > https://codereview.chromium.org/302603012/diff/60001/content/common/view_mess... > File content/common/view_messages.h (right): > > https://codereview.chromium.org/302603012/diff/60001/content/common/view_mess... > content/common/view_messages.h:689: std::set<int> /* exceptions */) > I'm still not particularly comfortable about this exceptions list. A developer > coming in to add new zoom functionality would not have an understanding of why > there's an exception list. I would think this can be done through appropriate comments? Below, the message names are chosen to indicate what's going on ... one changes the zoom level for any renderer with the indicated URL, the other just changes it for the target view. > Also, it's not clear how SetZoomLevelForView > interacts with SetZoomLevelForCurrentURL. > > If the exception bit is renderer-side, set form the other IPC (renaming the > other IPC to highlight the terminology "TemporaryZoomLevel"), then it becomes > clear how these two IPCs mesh together. I'll make the change, but I think moving information that affects what will and won't change its zoom level to a place outside HostZoomMap will make it harder to understand the operation, but it's your call.
PTAL
Two main thoughts: 1. I think a couple of the IPCs can be merged into one because they are tightly coupled. 2. It doesn't make sense to have a TemporaryZoomLevels vector and iterate over the vector all over the place. Making this a map ought to simplify the code significantly. 3. I really wish manual mode was baked into the content API as it would probably make the API easier to understand: no separate SendTemporaryZoomLevelChanges API, but we need to run this by John. I'll have an offline conversation about that with him. https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:246: NavigationEntry* entry = It looks like this is only used in the else clause. Move this down to the else clause. https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:266: SendTemporaryZoomLevelChange(render_process_id, render_view_id); This looks like the only call site for SetZoomLevelForView: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:282: void HostZoomMapImpl::SetUsesTemporaryZoomLevel( Do we need this method? It seems unnecessary if we use SetTemporaryZoomLevel and ClearTemporaryZoomLevel. https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:317: std::string GetHostFromProcessView(int render_process_id, int render_view_id) { DCHECK to make sure this is only called on the UI thread. https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:347: temporary_zoom_levels_[i].zoom_level = level; This becomes trivial once it's a map. https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:358: host->Send(new ViewMsg_SetUsesTemporaryZoomLevel(render_view_id, true)); ViewMsg_SetTemporaryZoomLevel(render_view_id, true, level); https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:391: temporary_zoom_levels_[i].render_view_id == render_view_id) { With the map, the above becomes trivial. https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:398: host->Send(new ViewMsg_SetUsesTemporaryZoomLevel(render_view_id, false)); ViewMsg_SetTemporaryZoomLevel(render_view_id, false, /* grab the host zoom level here to reset behavior */); https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:404: void HostZoomMapImpl::SendTemporaryZoomLevelChange(int render_process_id, I wish we didn't have to expose this publicly. Ideally manual would be another bit we set in the renderer so that the behavior sticks for the lifetime of the renderer. Ignore this comment for now, but we can get John's feedback here. https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:418: } With the map, the above becomes trivial. https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:425: host->Send(new ViewMsg_SetZoomLevelForView(render_view_id, level)); ViewMsg_SetTemporaryZoomLevel(render_view_id, true, level); https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... File content/browser/host_zoom_map_impl.h (right): https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.h:50: virtual void SetTemporaryZoomLevel(int render_process_id, Alphabetize. https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.h:137: TemporaryZoomLevels temporary_zoom_levels_; This is a pretty bad data structure to use as we're making temporary zoom a more prominent feature by exposing an extension API. proposal: struct RenderViewKey { int render_process_id; int render_view_id; RenderViewKey(int render_process_id, int render_view_id) : render_process_id(render_process_id), render_view_id(render_view_id) {} bool operator<(const RenderViewKey& other) const { return render_process_id < other.render_process_id || ((render_process_id == other.render_process_id) && (render_view_id < other.render_view_id)); } }; typedef std::map<RenderViewKey, double> TemporaryZoomMap; https://codereview.chromium.org/302603012/diff/80001/content/common/view_mess... File content/common/view_messages.h (right): https://codereview.chromium.org/302603012/diff/80001/content/common/view_mess... content/common/view_messages.h:695: IPC_MESSAGE_ROUTED1(ViewMsg_SetZoomLevelForView, These two IPCs are tightly coupled (see comments in host_zoom_map_impl. It makes sense to merge them. https://codereview.chromium.org/302603012/diff/80001/content/public/browser/h... File content/public/browser/host_zoom_map.h (right): https://codereview.chromium.org/302603012/diff/80001/content/public/browser/h... content/public/browser/host_zoom_map.h:88: // Returns all non-temporary zoom levels. Can only be called on any thread. Remove only. https://codereview.chromium.org/302603012/diff/80001/content/public/renderer/... File content/public/renderer/render_view.h (right): https://codereview.chromium.org/302603012/diff/80001/content/public/renderer/... content/public/renderer/render_view.h:130: virtual bool UsesTemporaryZoomLevel() const = 0; Is there a need to expose this to the content API? If not, remove. https://codereview.chromium.org/302603012/diff/80001/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/302603012/diff/80001/content/renderer/render_... content/renderer/render_view_impl.cc:1963: bool RenderViewImpl::UsesTemporaryZoomLevel() const { This doesn't need to be in the content API. https://codereview.chromium.org/302603012/diff/80001/content/renderer/render_... content/renderer/render_view_impl.cc:1969: uses_temporary_zoom_level_ = uses_temporary_zoom_level; Move this trivial accessor to the render_view_impl.h file. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Access... https://codereview.chromium.org/302603012/diff/80001/content/renderer/render_... content/renderer/render_view_impl.cc:2681: uses_temporary_zoom_level_ = uses_temporary_zoom_level; As I mentioned elsewhere, I think we should be merging the two IPCs since they are very tightly coupled. https://codereview.chromium.org/302603012/diff/80001/content/renderer/render_... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/302603012/diff/80001/content/renderer/render_... content/renderer/render_view_impl.h:514: virtual bool UsesTemporaryZoomLevel() const OVERRIDE; Duplicate comment: This doesn't need to be virtual nor a part of the content API.
I'll start working on the changes, but I don't think I agree with them all (see questions/comments below), and so will hold off on those until I get a reply on them. https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:246: NavigationEntry* entry = On 2014/06/10 20:05:13, Fady Samuel wrote: > It looks like this is only used in the else clause. Move this down to the else > clause. Done. https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:347: temporary_zoom_levels_[i].zoom_level = level; On 2014/06/10 20:05:13, Fady Samuel wrote: > This becomes trivial once it's a map. Done. https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:358: host->Send(new ViewMsg_SetUsesTemporaryZoomLevel(render_view_id, true)); On 2014/06/10 20:05:13, Fady Samuel wrote: > ViewMsg_SetTemporaryZoomLevel(render_view_id, true, level); But this will force us to send a level value for manual mode, where it is irrelevant. I don't think that makes sense. https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:391: temporary_zoom_levels_[i].render_view_id == render_view_id) { On 2014/06/10 20:05:13, Fady Samuel wrote: > With the map, the above becomes trivial. Done. https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:404: void HostZoomMapImpl::SendTemporaryZoomLevelChange(int render_process_id, On 2014/06/10 20:05:13, Fady Samuel wrote: > I wish we didn't have to expose this publicly. Ideally manual would be another > bit we set in the renderer so that the behavior sticks for the lifetime of the > renderer. Ignore this comment for now, but we can get John's feedback here. Done. https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:418: } On 2014/06/10 20:05:13, Fady Samuel wrote: > With the map, the above becomes trivial. Done. https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:425: host->Send(new ViewMsg_SetZoomLevelForView(render_view_id, level)); On 2014/06/10 20:05:13, Fady Samuel wrote: > ViewMsg_SetTemporaryZoomLevel(render_view_id, true, level); See my previous comment about combining these. https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... File content/browser/host_zoom_map_impl.h (right): https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.h:50: virtual void SetTemporaryZoomLevel(int render_process_id, On 2014/06/10 20:05:13, Fady Samuel wrote: > Alphabetize. Everything, or just the newly added stuff? https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.h:137: TemporaryZoomLevels temporary_zoom_levels_; On 2014/06/10 20:05:13, Fady Samuel wrote: > This is a pretty bad data structure to use as we're making temporary zoom a more > prominent feature by exposing an extension API. > > proposal: > > struct RenderViewKey { > int render_process_id; > int render_view_id; > RenderViewKey(int render_process_id, int render_view_id) > : render_process_id(render_process_id), > render_view_id(render_view_id) {} > bool operator<(const RenderViewKey& other) const { > return render_process_id < other.render_process_id || > ((render_process_id == other.render_process_id) && > (render_view_id < other.render_view_id)); > } > }; > > typedef std::map<RenderViewKey, double> TemporaryZoomMap; Done. https://codereview.chromium.org/302603012/diff/80001/content/common/view_mess... File content/common/view_messages.h (right): https://codereview.chromium.org/302603012/diff/80001/content/common/view_mess... content/common/view_messages.h:695: IPC_MESSAGE_ROUTED1(ViewMsg_SetZoomLevelForView, On 2014/06/10 20:05:14, Fady Samuel wrote: > These two IPCs are tightly coupled (see comments in host_zoom_map_impl. It makes > sense to merge them. Except then we are forced to set a level when in manual mode, which we don't really want to do. That's why I kept them separate. https://codereview.chromium.org/302603012/diff/80001/content/public/browser/h... File content/public/browser/host_zoom_map.h (right): https://codereview.chromium.org/302603012/diff/80001/content/public/browser/h... content/public/browser/host_zoom_map.h:88: // Returns all non-temporary zoom levels. Can only be called on any thread. On 2014/06/10 20:05:14, Fady Samuel wrote: > Remove only. Done. https://codereview.chromium.org/302603012/diff/80001/content/public/renderer/... File content/public/renderer/render_view.h (right): https://codereview.chromium.org/302603012/diff/80001/content/public/renderer/... content/public/renderer/render_view.h:130: virtual bool UsesTemporaryZoomLevel() const = 0; On 2014/06/10 20:05:14, Fady Samuel wrote: > Is there a need to expose this to the content API? If not, remove. See comment in render_view_impl.h https://codereview.chromium.org/302603012/diff/80001/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/302603012/diff/80001/content/renderer/render_... content/renderer/render_view_impl.cc:1969: uses_temporary_zoom_level_ = uses_temporary_zoom_level; On 2014/06/10 20:05:14, Fady Samuel wrote: > Move this trivial accessor to the render_view_impl.h file. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Access... Done. https://codereview.chromium.org/302603012/diff/80001/content/renderer/render_... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/302603012/diff/80001/content/renderer/render_... content/renderer/render_view_impl.h:514: virtual bool UsesTemporaryZoomLevel() const OVERRIDE; On 2014/06/10 20:05:14, Fady Samuel wrote: > Duplicate comment: This doesn't need to be virtual nor a part of the content > API. If it's not part of the content API, then I need to convert a RenderView* to RenderViewImpl* in RenderThreadImpl*, and I've had push-back on this before.
PTAL https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:282: void HostZoomMapImpl::SetUsesTemporaryZoomLevel( On 2014/06/10 20:05:13, Fady Samuel wrote: > Do we need this method? It seems unnecessary if we use SetTemporaryZoomLevel and > ClearTemporaryZoomLevel. Done. https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:317: std::string GetHostFromProcessView(int render_process_id, int render_view_id) { On 2014/06/10 20:05:13, Fady Samuel wrote: > DCHECK to make sure this is only called on the UI thread. Done. https://codereview.chromium.org/302603012/diff/80001/content/browser/host_zoo... content/browser/host_zoom_map_impl.cc:358: host->Send(new ViewMsg_SetUsesTemporaryZoomLevel(render_view_id, true)); On 2014/06/10 20:18:54, wjmaclean wrote: > On 2014/06/10 20:05:13, Fady Samuel wrote: > > ViewMsg_SetTemporaryZoomLevel(render_view_id, true, level); > > But this will force us to send a level value for manual mode, where it is > irrelevant. I don't think that makes sense. Now that content will have no knowledge of manual mode, we'll merge these. https://codereview.chromium.org/302603012/diff/80001/content/renderer/render_... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/302603012/diff/80001/content/renderer/render_... content/renderer/render_view_impl.cc:1969: uses_temporary_zoom_level_ = uses_temporary_zoom_level; On 2014/06/10 20:05:14, Fady Samuel wrote: > Move this trivial accessor to the render_view_impl.h file. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Access... Removed ... we're not using it at present anyways.
This looks really good. Just a few small changes and I'll happily sign off on it. :-) https://codereview.chromium.org/302603012/diff/100001/content/browser/host_zo... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/302603012/diff/100001/content/browser/host_zo... content/browser/host_zoom_map_impl.cc:278: return temporary_zoom_levels_.find(key) != temporary_zoom_levels_.end(); Use ContainsKey: https://code.google.com/p/chromium/codesearch#chromium/src/base/stl_util.h&q=... https://codereview.chromium.org/302603012/diff/100001/content/browser/host_zo... content/browser/host_zoom_map_impl.cc:285: TemporaryZoomLevels::const_iterator it = temporary_zoom_levels_.find(key); for readability: if (!ContainsKey(temporary_zoom_levels_, key)) return 0; return temporary_zoom_levels_[key]; https://codereview.chromium.org/302603012/diff/100001/content/browser/host_zo... content/browser/host_zoom_map_impl.cc:324: if (it == temporary_zoom_levels_.end()) Use ContainsKey. https://codereview.chromium.org/302603012/diff/100001/content/browser/host_zo... content/browser/host_zoom_map_impl.cc:367: // TODO(wjmaclean) Do we need to lookup the appropriate per-origin Do this now? This comment and the one below seem to be referring to the same thing. https://codereview.chromium.org/302603012/diff/100001/content/browser/host_zo... content/browser/host_zoom_map_impl.cc:374: // TODO(wjmaclean) Need to see if there's a mapped host zoom level to use. Please do this now? Would calling GetZoomLevelForHost(GetHostFromProcessView(render_process_id, render_view_id)) instead of GetDefaultZoomLevel() do the trick? https://codereview.chromium.org/302603012/diff/100001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/302603012/diff/100001/content/renderer/render... content/renderer/render_thread_impl.cc:183: double zoom_level) Unnecessary change. Please revert to avoid code churn. https://codereview.chromium.org/302603012/diff/100001/content/renderer/render... content/renderer/render_thread_impl.cc:1198: void RenderThreadImpl::OnSetZoomLevelForCurrentURL( Unnecessary change, please revert to avoid code churn.
https://codereview.chromium.org/302603012/diff/100001/content/browser/host_zo... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/302603012/diff/100001/content/browser/host_zo... content/browser/host_zoom_map_impl.cc:278: return temporary_zoom_levels_.find(key) != temporary_zoom_levels_.end(); On 2014/06/12 18:49:48, Fady Samuel wrote: > Use ContainsKey: > https://code.google.com/p/chromium/codesearch#chromium/src/base/stl_util.h&q=... Done. https://codereview.chromium.org/302603012/diff/100001/content/browser/host_zo... content/browser/host_zoom_map_impl.cc:285: TemporaryZoomLevels::const_iterator it = temporary_zoom_levels_.find(key); On 2014/06/12 18:49:48, Fady Samuel wrote: > for readability: > > if (!ContainsKey(temporary_zoom_levels_, key)) > return 0; > > return temporary_zoom_levels_[key]; Done. We don't worry about the cost of two lookups on key here? BTW, we can't do the return quite like this, as [] isn't const. https://codereview.chromium.org/302603012/diff/100001/content/browser/host_zo... content/browser/host_zoom_map_impl.cc:324: if (it == temporary_zoom_levels_.end()) On 2014/06/12 18:49:48, Fady Samuel wrote: > Use ContainsKey. Actually, we can just assign using [] since it doesn't matter if it already contains the key, it will when we're done! https://codereview.chromium.org/302603012/diff/100001/content/browser/host_zo... content/browser/host_zoom_map_impl.cc:367: // TODO(wjmaclean) Do we need to lookup the appropriate per-origin On 2014/06/12 18:49:48, Fady Samuel wrote: > Do this now? This comment and the one below seem to be referring to the same > thing. Done. https://codereview.chromium.org/302603012/diff/100001/content/browser/host_zo... content/browser/host_zoom_map_impl.cc:374: // TODO(wjmaclean) Need to see if there's a mapped host zoom level to use. On 2014/06/12 18:49:48, Fady Samuel wrote: > Please do this now? Would calling > GetZoomLevelForHost(GetHostFromProcessView(render_process_id, render_view_id)) > instead of GetDefaultZoomLevel() do the trick? Yeah, probably ... I was still sort of deciding about what to do here, but that's reasonable. https://codereview.chromium.org/302603012/diff/100001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/302603012/diff/100001/content/renderer/render... content/renderer/render_thread_impl.cc:183: double zoom_level) On 2014/06/12 18:49:48, Fady Samuel wrote: > Unnecessary change. Please revert to avoid code churn. Done. https://codereview.chromium.org/302603012/diff/100001/content/renderer/render... content/renderer/render_thread_impl.cc:1198: void RenderThreadImpl::OnSetZoomLevelForCurrentURL( On 2014/06/12 18:49:48, Fady Samuel wrote: > Unnecessary change, please revert to avoid code churn. Done.
lgtm with nits. https://codereview.chromium.org/302603012/diff/120001/content/browser/host_zo... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/302603012/diff/120001/content/browser/host_zo... content/browser/host_zoom_map_impl.cc:243: SetTemporaryZoomLevel(render_process_id, nit: Unnecessary change, revert. https://codereview.chromium.org/302603012/diff/120001/content/browser/host_zo... content/browser/host_zoom_map_impl.cc:361: // TODO(wjmaclean) Do we need to lookup the appropriate per-origin nit: You've addressed this right? You can remove this TODO. https://codereview.chromium.org/302603012/diff/120001/content/browser/host_zo... content/browser/host_zoom_map_impl.cc:368: // TODO(wjmaclean) Need to see if there's a mapped host zoom level to use. nit: You've addressed this right? This TODO can go away.
https://codereview.chromium.org/302603012/diff/120001/content/common/view_mes... File content/common/view_messages.h (right): https://codereview.chromium.org/302603012/diff/120001/content/common/view_mes... content/common/view_messages.h:684: // Render views in the exception list do not have their levels changed. Remove this comment, as its no longer relevant.
https://codereview.chromium.org/302603012/diff/120001/content/common/view_mes... File content/common/view_messages.h (right): https://codereview.chromium.org/302603012/diff/120001/content/common/view_mes... content/common/view_messages.h:684: // Render views in the exception list do not have their levels changed. Remove this comment, as its no longer relevant.
https://codereview.chromium.org/302603012/diff/120001/content/browser/host_zo... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/302603012/diff/120001/content/browser/host_zo... content/browser/host_zoom_map_impl.cc:243: SetTemporaryZoomLevel(render_process_id, On 2014/06/12 19:42:44, Fady Samuel wrote: > nit: Unnecessary change, revert. Done. https://codereview.chromium.org/302603012/diff/120001/content/browser/host_zo... content/browser/host_zoom_map_impl.cc:361: // TODO(wjmaclean) Do we need to lookup the appropriate per-origin On 2014/06/12 19:42:44, Fady Samuel wrote: > nit: You've addressed this right? You can remove this TODO. D'oh! Removed. https://codereview.chromium.org/302603012/diff/120001/content/browser/host_zo... content/browser/host_zoom_map_impl.cc:368: // TODO(wjmaclean) Need to see if there's a mapped host zoom level to use. On 2014/06/12 19:42:44, Fady Samuel wrote: > nit: You've addressed this right? This TODO can go away. I 'rewrote' this one, then forgot to write the file before upload. Done. https://codereview.chromium.org/302603012/diff/120001/content/common/view_mes... File content/common/view_messages.h (right): https://codereview.chromium.org/302603012/diff/120001/content/common/view_mes... content/common/view_messages.h:684: // Render views in the exception list do not have their levels changed. On 2014/06/12 19:43:18, Fady Samuel wrote: > Remove this comment, as its no longer relevant. Done.
https://codereview.chromium.org/302603012/diff/140001/content/browser/host_zo... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/302603012/diff/140001/content/browser/host_zo... content/browser/host_zoom_map_impl.cc:289: namespace { nit: in general, there's usually one anonymous namespace at the top of the file https://codereview.chromium.org/302603012/diff/140001/content/browser/host_zo... content/browser/host_zoom_map_impl.cc:299: DCHECK(web_contents); this is unnecessary. in release builds, it does nothing. in debug, the crash in the next line is just as informational https://codereview.chromium.org/302603012/diff/140001/content/browser/host_zo... content/browser/host_zoom_map_impl.cc:324: DCHECK(host); ditto https://codereview.chromium.org/302603012/diff/140001/content/browser/host_zo... File content/browser/host_zoom_map_impl.h (right): https://codereview.chromium.org/302603012/diff/140001/content/browser/host_zo... content/browser/host_zoom_map_impl.h:109: typedef std::map<RenderViewKey, double> TemporaryZoomLevels; this change is extraneous, why do you need it now? i'm assuming the original code was written that way because very few pages are zoomed, so a vector is enough. have you seen a reason that causes this assumption to be made not valid anymore? https://codereview.chromium.org/302603012/diff/140001/content/public/browser/... File content/public/browser/host_zoom_map.h (right): https://codereview.chromium.org/302603012/diff/140001/content/public/browser/... content/public/browser/host_zoom_map.h:112: // Returns whether the view manages its zoom level independently of other tabs nit: s/tabs/views (content doesn't know about tabs) https://codereview.chromium.org/302603012/diff/140001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/302603012/diff/140001/content/renderer/render... content/renderer/render_view_impl.cc:677: next_snapshot_id_(0) {} i believe this is something that is supposed to be fixed in clang format but we might not have it yet. undo this
https://codereview.chromium.org/302603012/diff/140001/content/browser/host_zo... File content/browser/host_zoom_map_impl.h (right): https://codereview.chromium.org/302603012/diff/140001/content/browser/host_zo... content/browser/host_zoom_map_impl.h:109: typedef std::map<RenderViewKey, double> TemporaryZoomLevels; On 2014/06/12 21:35:52, jam wrote: > this change is extraneous, why do you need it now? i'm assuming the original > code was written that way because very few pages are zoomed, so a vector is > enough. have you seen a reason that causes this assumption to be made not valid > anymore? I suggested this change in code review: 1. It makes the code somewhat easier to read as we don't need an iteration of the vector for every operation on temporary zoom. It also results in fewer lines of code. See patchset #4 where it used a vector instead: there's a lot more code and so I felt readability suffered a bit. 2. With the chrome extension zoom API, I would expect we would end up with more pages than currently that are zoomed via temporary zoom. I feel point #1 is more significant though. It's more of a readability win than a performance issue.
lgtm https://codereview.chromium.org/302603012/diff/140001/content/browser/host_zo... File content/browser/host_zoom_map_impl.h (right): https://codereview.chromium.org/302603012/diff/140001/content/browser/host_zo... content/browser/host_zoom_map_impl.h:109: typedef std::map<RenderViewKey, double> TemporaryZoomLevels; On 2014/06/12 21:43:49, Fady Samuel wrote: > On 2014/06/12 21:35:52, jam wrote: > > this change is extraneous, why do you need it now? i'm assuming the original > > code was written that way because very few pages are zoomed, so a vector is > > enough. have you seen a reason that causes this assumption to be made not > valid > > anymore? > > I suggested this change in code review: > > 1. It makes the code somewhat easier to read as we don't need an iteration of > the vector for every operation on temporary zoom. It also results in fewer lines > of code. See patchset #4 where it used a vector instead: there's a lot more code > and so I felt readability suffered a bit. > > 2. With the chrome extension zoom API, I would expect we would end up with more > pages than currently that are zoomed via temporary zoom. > > I feel point #1 is more significant though. It's more of a readability win than > a performance issue. ok. i think this comes down to personal preference, but i dont feel too strongly
Fixed nits. +palmer@ for OWNERS approval on view_messages.h https://codereview.chromium.org/302603012/diff/140001/content/browser/host_zo... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/302603012/diff/140001/content/browser/host_zo... content/browser/host_zoom_map_impl.cc:289: namespace { On 2014/06/12 21:35:52, jam wrote: > nit: in general, there's usually one anonymous namespace at the top of the file Done. https://codereview.chromium.org/302603012/diff/140001/content/browser/host_zo... content/browser/host_zoom_map_impl.cc:299: DCHECK(web_contents); On 2014/06/12 21:35:52, jam wrote: > this is unnecessary. in release builds, it does nothing. in debug, the crash in > the next line is just as informational Done. https://codereview.chromium.org/302603012/diff/140001/content/browser/host_zo... content/browser/host_zoom_map_impl.cc:324: DCHECK(host); On 2014/06/12 21:35:52, jam wrote: > ditto Done. https://codereview.chromium.org/302603012/diff/140001/content/public/browser/... File content/public/browser/host_zoom_map.h (right): https://codereview.chromium.org/302603012/diff/140001/content/public/browser/... content/public/browser/host_zoom_map.h:112: // Returns whether the view manages its zoom level independently of other tabs On 2014/06/12 21:35:52, jam wrote: > nit: s/tabs/views (content doesn't know about tabs) Done. https://codereview.chromium.org/302603012/diff/140001/content/public/browser/... content/public/browser/host_zoom_map.h:137: Subscription; Removing this accidental change also. https://codereview.chromium.org/302603012/diff/140001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/302603012/diff/140001/content/renderer/render... content/renderer/render_view_impl.cc:677: next_snapshot_id_(0) {} On 2014/06/12 21:35:52, jam wrote: > i believe this is something that is supposed to be fixed in clang format but we > might not have it yet. undo this Done.
IPC security LGTM, although I do think it's better to go with GURLs than with arbitrary strings. My security instincts are to always deal with origins *only*, and not try to carve out other sub-concepts. But that may not be appropriate here. https://codereview.chromium.org/302603012/diff/160001/content/browser/host_zo... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/302603012/diff/160001/content/browser/host_zo... content/browser/host_zoom_map_impl.cc:192: SendZoomLevelChange(std::string(), host, level); Empty scheme? Why? It seems like this should always be tied to a complete origin (scheme, host, port). https://codereview.chromium.org/302603012/diff/160001/content/browser/host_zo... File content/browser/host_zoom_map_impl.h (right): https://codereview.chromium.org/302603012/diff/160001/content/browser/host_zo... content/browser/host_zoom_map_impl.h:38: virtual bool HasZoomLevel(const std::string& scheme, Seems like a GURL is a better choice here: It's a more constrained type. https://codereview.chromium.org/302603012/diff/160001/content/browser/host_zo... content/browser/host_zoom_map_impl.h:115: void SendZoomLevelChange(const std::string& scheme, Same comment here: Prefer the more constrained GURL instead of 2 arbitrary strings.
Bug filed, comments added. https://codereview.chromium.org/302603012/diff/160001/content/browser/host_zo... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/302603012/diff/160001/content/browser/host_zo... content/browser/host_zoom_map_impl.cc:192: SendZoomLevelChange(std::string(), host, level); On 2014/06/13 17:47:01, Chromium Palmer wrote: > Empty scheme? Why? It seems like this should always be tied to a complete origin > (scheme, host, port). Filed bug to investigate: crbug.com/384486. https://codereview.chromium.org/302603012/diff/160001/content/browser/host_zo... File content/browser/host_zoom_map_impl.h (right): https://codereview.chromium.org/302603012/diff/160001/content/browser/host_zo... content/browser/host_zoom_map_impl.h:38: virtual bool HasZoomLevel(const std::string& scheme, On 2014/06/13 17:47:01, Chromium Palmer wrote: > Seems like a GURL is a better choice here: It's a more constrained type. Filed crbug.com/384486 to investigate the feasibility of this. Comment added to code. https://codereview.chromium.org/302603012/diff/160001/content/browser/host_zo... content/browser/host_zoom_map_impl.h:115: void SendZoomLevelChange(const std::string& scheme, On 2014/06/13 17:47:01, Chromium Palmer wrote: > Same comment here: Prefer the more constrained GURL instead of 2 arbitrary > strings. Ditto.
The CQ bit was checked by wjmaclean@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/302603012/180001
Message was sent while issue was closed.
Change committed as 277153 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
