OOP PDF - Add support for "zoom" open pdf parameter

OOP PDF - Add support for "zoom" open pdf parameter

This patch adds support for zoom open pdf parameter. If user
opens pdf with zoom parameter in url then pdf should be opened
at zoom level mentioned by user.

BUG=64309, 319910
Committed: https://crrev.com/27548ef89c672922f9c3888183c59d48ef2e7ae0
Cr-Commit-Position: refs/heads/master@{#293545}

[Nikhil, 2014-07-26 14:22:36]

PTAL at my proposed solution to support zoom parameter mentioned in
specification for "Open PDF Parameters".

If you've any review comments, then please let me know. I'll incorporate them.

Thanks!

(please note that I'll refactor GetInitialPage() if approach looks fine)

[raymes, 2014-07-28 23:56:46]

On 2014/07/26 14:22:36, Nikhil wrote:
> PTAL at my proposed solution to support zoom parameter mentioned in
> specification for "Open PDF Parameters".
> 
> If you've any review comments, then please let me know. I'll incorporate them.
> 
> Thanks!
> 
> (please note that I'll refactor GetInitialPage() if approach looks fine)

Hey Nikhil, I'm ok with this but one problem is that we will soon be switching
to the out of process PDF plugin and this will stop working at that point. 

In the out of process PDF case, this could be implemented inside of
chrome/browser/resources/pdf/pdf.js

You can run the OOP plugin by passing --out-of-process-pdf.

[Nikhil, 2014-07-29 05:58:21]

On 2014/07/28 23:56:46, raymes wrote:
> On 2014/07/26 14:22:36, Nikhil wrote:
> > PTAL at my proposed solution to support zoom parameter mentioned in
> > specification for "Open PDF Parameters".
> > 
> > If you've any review comments, then please let me know. I'll incorporate
them.
> > 
> > Thanks!
> > 
> > (please note that I'll refactor GetInitialPage() if approach looks fine)
> 
> Hey Nikhil, I'm ok with this but one problem is that we will soon be switching
> to the out of process PDF plugin and this will stop working at that point. 
> 
> In the out of process PDF case, this could be implemented inside of
> chrome/browser/resources/pdf/pdf.js
> 
> You can run the OOP plugin by passing --out-of-process-pdf.

Hey Raymes, Thanks a lot for taking a look at this.

If out of process (OOP) release isn't immediate, then maybe we can go ahead with
this patch? And in the meantime, I'll explore OOP and try to prepare patch so
that we can support this feature upon OOP release as well.

Please let me know what do you think.

Thanks!

[raymes, 2014-07-30 01:10:15]

The code looks like it should work to me but my gut feeling is to not add any
new features in this file due to its poor code health and wait for the OOP
plugin. I fear introducing new bugs here and there are no tests.

For example we already have code for parsing the initial page out of the URL. If
we were implementing this properly we would refactor things to parse out the
key/value pairs into a map. 

Is there an urgent need for this feature? jam@ could you chime in with your
thoughts?

https://codereview.chromium.org/420063002/diff/1/pdf/instance.cc
File pdf/instance.cc (right):

https://codereview.chromium.org/420063002/diff/1/pdf/instance.cc#newcode2328
pdf/instance.cc:2328: void Instance::HandleZoomParameter(const std::string&
value) {
nit: value->url

https://codereview.chromium.org/420063002/diff/1/pdf/instance.cc#newcode2339
pdf/instance.cc:2339: double zoom_factor = (scale_value / 100);
no need for ()

https://codereview.chromium.org/420063002/diff/1/pdf/instance.cc#newcode2362
pdf/instance.cc:2362: #else
You probably don't need this ifdef. I don't think we build for NaCl anymore.

[Lei Zhang, 2014-07-30 02:43:21]

https://codereview.chromium.org/420063002/diff/1/pdf/instance.cc
File pdf/instance.cc (right):

https://codereview.chromium.org/420063002/diff/1/pdf/instance.cc#newcode1511
pdf/instance.cc:1511: HandleOpenParameters(url_);
Please note there is another call to GetInitialPage() in this file.

https://codereview.chromium.org/420063002/diff/1/pdf/instance.cc#newcode2359
pdf/instance.cc:2359: void Instance::HandleOpenParameters(const std::string&
url) {
Isn't this duplicating a good chunk of the code right below in GetInitialPage()
?

https://codereview.chromium.org/420063002/diff/1/pdf/instance.cc#newcode2362
pdf/instance.cc:2362: #else
On 2014/07/30 01:10:15, raymes wrote:
> You probably don't need this ifdef. I don't think we build for NaCl anymore.

raymes: If that is indeed the case, would you mind cleaning it all up in a
separate CL? (In addition to not adding any new ones here.)

https://codereview.chromium.org/420063002/diff/1/pdf/instance.cc#newcode2371
pdf/instance.cc:2371: for (size_t i = 0; i < fragments.size(); ++i) {
What you really want to do is to scan the fragments once. Then find the last
instance of #zoom and #page, and then just handle those in order.

[Nikhil, 2014-07-30 03:11:31]

Thanks for taking a look at this!

Yes, there is code duplication with GetInitialPage(). But like I already
mentioned in my first comment, I'll refactor it accordingly if we are to support
this feature.

Just waiting for the go-ahead to get to it :)

[jam, 2014-07-30 20:40:23]

On 2014/07/30 01:10:15, raymes wrote:
> The code looks like it should work to me but my gut feeling is to not add any
> new features in this file due to its poor code health and wait for the OOP
> plugin. I fear introducing new bugs here and there are no tests.
> 
> For example we already have code for parsing the initial page out of the URL.
If
> we were implementing this properly we would refactor things to parse out the
> key/value pairs into a map. 
> 
> Is there an urgent need for this feature? jam@ could you chime in with your
> thoughts?

I defer to you and other owners in src/pdf, I'm quite overloaded with the code
yellow and you're touching this code more than me now :)

The only thing I'll say is that given that this bug has been open for 4 years,
there's not much urgency to fixing it now before waiting for the OOP code.

[raymes, 2014-07-31 04:34:00]

Ok - in that case I'd rather hold off on this, but if thestig@ wants to see it
landed, that's also fine with me :)

[Lei Zhang, 2014-07-31 19:14:35]

On 2014/07/31 04:34:00, raymes wrote:
> Ok - in that case I'd rather hold off on this, but if thestig@ wants to see it
> landed, that's also fine with me :)

Well, I was waiting for OOP PDF before working on the bug, but if Nikhil is
already working on it, then sure, why not?

[Nikhil, 2014-08-04 09:34:32]

thestig@ raymes@ - Shall I go ahead and re-work based on review comments?

[Lei Zhang, 2014-08-04 18:06:44]

On 2014/08/04 09:34:32, Nikhil wrote:
> thestig@ raymes@ - Shall I go ahead and re-work based on review comments?

It's up to you. Please go ahead if you would like to.

[Nikhil, 2014-08-08 09:46:21]

On 2014/08/04 18:06:44, Lei Zhang wrote:
> On 2014/08/04 09:34:32, Nikhil wrote:
> > thestig@ raymes@ - Shall I go ahead and re-work based on review comments?
> 
> It's up to you. Please go ahead if you would like to.

great, I'll update the patch as per review comments. Thanks!

[Nikhil, 2014-08-13 13:41:29]

Small update - 

I've been tweaking around pdf.js, and I'm inclining towards going OOP way
(assuming release is in near future). I'm thinking of pulling out support for
"page" from out_of_process_instance.cc first and move it to pdf.js. And then
support "zoom" and others. Hope this makes sense.

I'm keeping this review open to revisit in future :)

[Nikhil, 2014-08-19 06:07:14]

On 2014/08/13 13:41:29, Nikhil wrote:
> Small update - 
> 
> I've been tweaking around pdf.js, and I'm inclining towards going OOP way
> (assuming release is in near future). I'm thinking of pulling out support for
> "page" from out_of_process_instance.cc first and move it to pdf.js. And then
> support "zoom" and others. Hope this makes sense.
> 
> I'm keeping this review open to revisit in future :)

raymes@ and thestig@ - I'll update this next, once the CL for "page" parameter
lands (yay!).

[Nikhil, 2014-08-19 15:09:42]

raymes@ - Could you please take a look at this?

Thanks!

https://codereview.chromium.org/420063002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/420063002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:282: this.viewport_.setZoom(zoomFactor);
setZoom() isn't working as intended. Is this not the correct way to use this?

[raymes, 2014-08-20 00:14:21]

https://codereview.chromium.org/420063002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/420063002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:282: this.viewport_.setZoom(zoomFactor);
On 2014/08/19 15:09:42, Nikhil wrote:
> setZoom() isn't working as intended. Is this not the correct way to use this?

This looks correct. What is it that isn't working? You might need to do some
debugging. Something may be changing the zoom after this call.

https://codereview.chromium.org/420063002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:297: * See http://crbug.com/64309 for
details.
nit: we don't need the crbug link,but a link to a spec document for Open PDF
params would be useful :)

https://codereview.chromium.org/420063002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:351: this.viewport_.position =
this.lastViewportPosition_;
It shouldn't make a difference at present but I think this line should be moved
up above handleOpenPDFParams.

[Lei Zhang, 2014-08-20 00:37:07]

https://codereview.chromium.org/420063002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/420063002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:287: var position = {x:
parseFloat(paramValueSplit[1]),
The Adobe open parameters document is rather vague on what units left/top are.
Do we need to do any unit conversion here?

https://codereview.chromium.org/420063002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:320: if ('zoom' in paramsDictionary)
I think order might also matter between page and zoom? Best to test with Acrobat
and see how it behaves.

One possible interpretation is:

(A) ?zoom=150,100,200&page=2 -> zoom to 150%, go to (100, 200), then go to page
2 -> ends up at top of page 2

vs

(B) ?page=2&zoom=150,100,200 -> go to page 2, zoom to 150%, go to (100, 200) ->
ends up in the middle of page 2

Other possible interpretations are we always do (A) or we always do (B).

[Nikhil, 2014-08-20 04:46:55]

https://codereview.chromium.org/420063002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/420063002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:282: this.viewport_.setZoom(zoomFactor);
On 2014/08/20 00:14:21, raymes wrote:
> On 2014/08/19 15:09:42, Nikhil wrote:
> > setZoom() isn't working as intended. Is this not the correct way to use
this?
> 
> This looks correct. What is it that isn't working? You might need to do some
> debugging. Something may be changing the zoom after this call.

It didn't work as intended since zoom didn't change to what was passed in the
parameter. But if I just attach debugger, pause, and continue execution then it
works fine. Maybe some sort of timing issue?

I wasn't sure if I was calling setZoom() correctly. I'll try to debug this more.

https://codereview.chromium.org/420063002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:287: var position = {x:
parseFloat(paramValueSplit[1]),
On 2014/08/20 00:37:07, Lei Zhang wrote:
> The Adobe open parameters document is rather vague on what units left/top are.
> Do we need to do any unit conversion here?

Yep, it's a bit vague on units. Unlike page which can only take integer values,
the Adobe document says scale, left, and top can be passed as integer or float
values. So I used parseFloat().

Also, do we need NaN check here to make it robust?

https://codereview.chromium.org/420063002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:320: if ('zoom' in paramsDictionary)
On 2014/08/20 00:37:07, Lei Zhang wrote:
> I think order might also matter between page and zoom? Best to test with
Acrobat
> and see how it behaves.
> 
> One possible interpretation is:
> 
> (A) ?zoom=150,100,200&page=2 -> zoom to 150%, go to (100, 200), then go to
page
> 2 -> ends up at top of page 2
> 
> vs
> 
> (B) ?page=2&zoom=150,100,200 -> go to page 2, zoom to 150%, go to (100, 200)
->
> ends up in the middle of page 2
> 
> Other possible interpretations are we always do (A) or we always do (B).

As I understand, page should always be handled before zoom. Both
#zoom=150,100,200&page=2 and #page=2&zoom=150,100,200 should first go to page 2
and then zoom.

Please see "Specifying parameters in a URL" section of Adobe document.

[Nikhil, 2014-08-21 11:36:50]

This is working as expected now. The combination of page and zoom parameters is
also working fine.

PTAL at my proposed solution.

Thanks!

https://codereview.chromium.org/420063002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/420063002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:282: this.viewport_.setZoom(zoomFactor);
On 2014/08/20 00:14:21, raymes wrote:
> On 2014/08/19 15:09:42, Nikhil wrote:
> > setZoom() isn't working as intended. Is this not the correct way to use
this?
> 
> This looks correct. What is it that isn't working? You might need to do some
> debugging. Something may be changing the zoom after this call.

It wasn't working because chrome.tabs.onZoomChange() listener was changing the
zoom back to 1. I've made a change for this. PTAL.

https://codereview.chromium.org/420063002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:297: * See http://crbug.com/64309 for
details.
On 2014/08/20 00:14:21, raymes wrote:
> nit: we don't need the crbug link,but a link to a spec document for Open PDF
> params would be useful :)

Done.

https://codereview.chromium.org/420063002/diff/20001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:351: this.viewport_.position =
this.lastViewportPosition_;
On 2014/08/20 00:14:21, raymes wrote:
> It shouldn't make a difference at present but I think this line should be
moved
> up above handleOpenPDFParams.

Done.

https://codereview.chromium.org/420063002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/420063002/diff/40001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:149: if ((zoomDelta > MIN_ZOOM_DELTA) &&
!this.setZoomInProgress_)
This was again calling setZoom() with older zoom factor even though we are in
between of zoom triggered by #zoom parameter.
 
For test.pdf#zoom=120
this.viewport_.zoom = 1.2
zoomChangeInfo.newZoomFactor = 1

Since the delta was greater than MIN_ZOOM_DELTA, setZoom() was again called
setting viewport_.zoom to 1 and canceling the effect of #zoom parameter.

Please let me know if this fix is acceptable. If so, I'd really like to move it
to separate CL to keep this CL only about 'zoom' param implementation :)

[raymes, 2014-08-22 03:52:21]

https://codereview.chromium.org/420063002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/420063002/diff/40001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:149: if ((zoomDelta > MIN_ZOOM_DELTA) &&
!this.setZoomInProgress_)
Hmm I'm not sure if this is completely correct. Basically it means the order of
zoom events isn't preserved. For example if I do the following:
1) Initiate a zoom through the plugin
2) Initiate a zoom from the browser

Then the affect of (2) might be ignored, which isn't what we want. 

The problem here is that you want to set an "initial zoom". Have a look in
Viewport.setDocumentDimensions(). This is where we set the initial zoom (look
inside the block which has 
if (initialDimensions) {...

So I think we should add a member to Viewport (Viewport.initialViewport) which
is used in setDocumentDimensions (rather than the current way it computes the
initial zoom). initialViewport could be a dictionary which contains the initial
position/page to scroll to as well, which could also be set in that method.

Then we could actually just call handleOpenPDFParams_ from the PDFViewer
constructor.

https://codereview.chromium.org/420063002/diff/40001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:297: * See
http://www.adobe.com/content/dam/Adobe/en/devnet/acrobat/pdfs/pdf_open_parame...
you'll have to split up the URL so it doesn't extend past 80chars :(

https://codereview.chromium.org/420063002/diff/40001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:325: },
This class is already quite large and I'd like to start pulling stuff out of it
for maintainability. Would you be able to pull out these two functions into a
separate class/file called OpenPDFParamsParser (or something to that effect).
Then you could have some code in the PDFViewer constructor which looks like
this:

var params_parser = new OpenPDFParamsParser(url);
viewport_.initialViewport = params_parser.viewport;  // this is just dictionary
with a few fields

And then in Viewport.setDocumentDimensions:

if (this.initialViewport.position)
  // scroll to position
else if (this.initialViewport.page)
  // scroll to page
else 
  // scroll to (0, 0)

if (this.initialViewport.zoom)
  // zoom to zoom
else
  ...

Does this make sense?

[Nikhil, 2014-08-22 16:18:54]

On 2014/08/22 03:52:21, raymes wrote:
> 
> Does this make sense?

Thanks raymes@ for taking a look at this!

It makes perfect sense to have a separate class/file to parse open PDF
parameters. Going forward, a few more methods will be added, so it's better to
introduce a separate class/file now itself. Here's how I see it based on useful
information that you provided -

[1] Introduce OpenPDFParamsParser class and move related methods to it. Add
basic infrastructure required to parse parameters and return them in dictionary.
Update PDFViewer constructor. (these changes are ready!)

[2] Update Viewport.setDocumentDimensions. I tried to modify it like you
suggested, but I couldn't make it work in my first attempt. The page neither
scrolled nor zoomed in. I think a few more changes might be needed.

[3] Revisit this patch and update it accordingly.

I'm of the opinion of doing these separately to keep things simple. But if you
think [1] and [2] should be combined then we can do that also.

Please let me know WDYT.

Thanks!

[raymes, 2014-08-25 02:36:21]

On 2014/08/22 16:18:54, Nikhil wrote:
> On 2014/08/22 03:52:21, raymes wrote:
> > 
> > Does this make sense?
> 
> Thanks raymes@ for taking a look at this!
> 
> It makes perfect sense to have a separate class/file to parse open PDF
> parameters. Going forward, a few more methods will be added, so it's better to
> introduce a separate class/file now itself. Here's how I see it based on
useful
> information that you provided -
> 
> [1] Introduce OpenPDFParamsParser class and move related methods to it. Add
> basic infrastructure required to parse parameters and return them in
dictionary.
> Update PDFViewer constructor. (these changes are ready!)
> 
> [2] Update Viewport.setDocumentDimensions. I tried to modify it like you
> suggested, but I couldn't make it work in my first attempt. The page neither
> scrolled nor zoomed in. I think a few more changes might be needed.

Ok, please take a look into it. When I had a quick look last week it seemed like
it would work but I didn't test it.

> 
> [3] Revisit this patch and update it accordingly.
> 
> I'm of the opinion of doing these separately to keep things simple. But if you
> think [1] and [2] should be combined then we can do that also.
> 
> Please let me know WDYT.
> 
> Thanks!

Sounds good - I don't mind whether you do it in one or two CLs. Thanks :)

[Nikhil, 2014-08-29 10:31:47]

PTAL at changes to support zoom open pdf parameter. 

I've updated the patch to use OpenPDFParamsParser class. Also, both page and
zoom parameters are working fine together.

PTAL. Thanks!

https://codereview.chromium.org/420063002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/420063002/diff/60001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:153: if ((zoomDelta > MIN_ZOOM_DELTA) &&
!this.setZoomInProgress_)
Should this be a part of this CL or a separate one? Maybe associate it with OOP
PDF Issue - 303491.

[raymes, 2014-09-01 01:04:58]

https://codereview.chromium.org/420063002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/420063002/diff/60001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:153: if ((zoomDelta > MIN_ZOOM_DELTA) &&
!this.setZoomInProgress_)
It's fine to keep it here.

https://codereview.chromium.org/420063002/diff/60001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:288: destinationPosition['y'] =
currentPosition.y + this.urlParams_.position.y;
Is this the way that adobe reader works (does it add the position param to the
page param)? I would have though it would either choose one or the other.

Maybe you could just write this as:
this.viewport_.position = {
  x: this.viewport_.position.x + this.urlParams_.position.x;
  y: this.viewport_.position.y + this.urlParams_.position.y;
};

[Nikhil, 2014-09-01 09:11:44]

raymes@ - PTAL at my response to your review comment. Thanks!

https://codereview.chromium.org/420063002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/420063002/diff/60001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:153: if ((zoomDelta > MIN_ZOOM_DELTA) &&
!this.setZoomInProgress_)
On 2014/09/01 01:04:58, raymes wrote:
> It's fine to keep it here.

Acknowledged.

https://codereview.chromium.org/420063002/diff/60001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:288: destinationPosition['y'] =
currentPosition.y + this.urlParams_.position.y;
On 2014/09/01 01:04:58, raymes wrote:
> Is this the way that adobe reader works (does it add the position param to the
> page param)? I would have though it would either choose one or the other.
> 
> Maybe you could just write this as:
> this.viewport_.position = {
>   x: this.viewport_.position.x + this.urlParams_.position.x;
>   y: this.viewport_.position.y + this.urlParams_.position.y;
> };
> 

Please consider the following use case -
example.com/test.pdf#page=2&zoom=120,150,150 (or #zoom=120,150,150&page=2)

The above should zoom the position (150,150) of page 2 to 120% zoom level. Same
behavior is observed with Adobe Reader as pdf viewer. I think Firefox doesn't
support zoom parameter as of now.

To support above behavior, current implementation is in the following way -
[1] Go to page if mentioned
[2] Adjust position if mentioned (using current position)
[3] Set zoom level if mentioned

Hmm, maybe we should factor in current position in [2] only if page param is
passed? I think it could be possible for authors to specify a position and
hyperlink it to navigate up/down in a big document, in which case I think
current logic will fail. Since it will always use _current_ position and add to
it. But, I can't seem to find such a pdf to test this. 

Please let me know WDYT.

[raymes, 2014-09-02 03:27:37]

https://codereview.chromium.org/420063002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/420063002/diff/60001/chrome/browser/resources...
chrome/browser/resources/pdf/pdf.js:288: destinationPosition['y'] =
currentPosition.y + this.urlParams_.position.y;
The way you have written it looks fine. But I think you can simplify the code as
I suggested in my previous comment.

[Nikhil, 2014-09-02 04:13:49]

On 2014/09/02 03:27:37, raymes wrote:
>
https://codereview.chromium.org/420063002/diff/60001/chrome/browser/resources...
> File chrome/browser/resources/pdf/pdf.js (right):
> 
>
https://codereview.chromium.org/420063002/diff/60001/chrome/browser/resources...
> chrome/browser/resources/pdf/pdf.js:288: destinationPosition['y'] =
> currentPosition.y + this.urlParams_.position.y;
> The way you have written it looks fine. But I think you can simplify the code
as
> I suggested in my previous comment.

raymes@ - I've already modified code as per your suggestion. Please see patch
set 5 -
https://codereview.chromium.org/420063002/diff2/60001:80001/chrome/browser/re...

[raymes, 2014-09-02 04:37:38]

lgtm

[Nikhil, 2014-09-02 05:42:51]

thestig@ - OWNERS review please.

[Nikhil, 2014-09-03 18:29:23]

thestig@ - <ping!> Could you please take a look at this for OWNERS review?
Thanks.

[Lei Zhang, 2014-09-05 16:08:00]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2014-09-05 16:08:01]

lgtm

[commit-bot: I haz the power, 2014-09-05 16:09:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/n.bansal@samsung.com/420063002/80001

[commit-bot: I haz the power, 2014-09-05 17:26:37]

Committed patchset #5 (id:80001) as 36a94623ab4e9e049d17fa7666e885b0d8db3ef1

[commit-bot: I haz the power, 2014-09-10 03:40:21]

Patchset 5 (id:??) landed as
https://crrev.com/27548ef89c672922f9c3888183c59d48ef2e7ae0
Cr-Commit-Position: refs/heads/master@{#293545}