Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
223363002:
Convert the code in pdf.js to an object (Closed)
|
Created:
6 years, 8 months ago by raymes Modified:
6 years, 8 months ago CC:
chromium-reviews, arv+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionConvert the code in pdf.js to an object
This is a refactoring of the code in pdf.js, moving the code into a PDFViewer object. This
will make it more testable and usable by other classes that will be added later.
BUG=303491
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262917
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 7
Patch Set 6 : #Messages
Total messages: 12 (0 generated)
Hey koz, PTAL. This looks kinda scary but it's purely just shifting code around into functions.
lgtm
+arv for OWNERS
https://codereview.chromium.org/223363002/diff/80001/chrome/browser/resources... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/223363002/diff/80001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:18: this.sizer_ = $('sizer'); This pattern is a bit strange. You are creating a constructor which implies that you want to be able to create multiple instances of PDFViewer, yet, you are hard coding to use a single element in the page. If I create 2 PDFViewer instances there will be conflicts. https://codereview.chromium.org/223363002/diff/80001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:76: if (!this.viewport_.documentHasScrollbars().x) { this is not lexically bound but dynamically bound. In this case this === document and document.viewport_ is not defined. Either us Function.prototype.bind or `var self = this` https://codereview.chromium.org/223363002/diff/80001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:197: * @param {Object} message a message event. Maybe use {MessageEvent} as the type?
https://codereview.chromium.org/223363002/diff/80001/chrome/browser/resources... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/223363002/diff/80001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:18: this.sizer_ = $('sizer'); On 2014/04/07 21:01:44, arv wrote: > This pattern is a bit strange. You are creating a constructor which implies that > you want to be able to create multiple instances of PDFViewer, yet, you are hard > coding to use a single element in the page. If I create 2 PDFViewer instances > there will be conflicts. > It is a bit weird. It would be less weird if the document was passed in as an argument on construction. Do you think it's worth making that change? I don't think that just because there is a constructor it implies we want to create multiple instances of it, just as singleton objects have constructors. Really the idea is that there should only be one of these things per document. And that is what we get here because the constructor isn't exposed outside of this file. What do you think? I added a comment suggestion that there should only be one of these per document :) https://codereview.chromium.org/223363002/diff/80001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:76: if (!this.viewport_.documentHasScrollbars().x) { Good catch :) I bound the function. https://codereview.chromium.org/223363002/diff/80001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:197: * @param {Object} message a message event. On 2014/04/07 21:01:44, arv wrote: > Maybe use {MessageEvent} as the type? Done.
Thanks!
https://codereview.chromium.org/223363002/diff/80001/chrome/browser/resources... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/223363002/diff/80001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:18: this.sizer_ = $('sizer'); On 2014/04/08 01:44:06, raymes wrote: > On 2014/04/07 21:01:44, arv wrote: > > This pattern is a bit strange. You are creating a constructor which implies > that > > you want to be able to create multiple instances of PDFViewer, yet, you are > hard > > coding to use a single element in the page. If I create 2 PDFViewer instances > > there will be conflicts. > > > > It is a bit weird. It would be less weird if the document was passed in as an > argument on construction. Do you think it's worth making that change? > > I don't think that just because there is a constructor it implies we want to > create multiple instances of it, just as singleton objects have constructors. > Really the idea is that there should only be one of these things per document. > And that is what we get here because the constructor isn't exposed outside of > this file. What do you think? > > I added a comment suggestion that there should only be one of these per document > :) Now that I look at it. Why is everything private? I think structuring this as an object just makes the code harder to maintain and you have to add all those pesky bind calls everywhere. I recommend rewriting this without OO. Wrap it in an IIFE to not introduce globals: (function() { 'use strict'; function onPasswordSubmitted(event) { ... } function updateProgress(progress) { progressBar.progress = progress; ... } ... var size = $('sizer'); var progressBar = $('progress-bar'); ... })();
I should have mentioned that there is a particular reason I'm making it an object. I'm creating an object in another CL which will call methods on this object. Although none of the methods are public now, I will be adding some. These methods will customize things related to the PDFViewer (like whether it is in print preview mode). It will be much easier to reason about with OO abstractions in place. I don't think that making it an object makes it harder to maintain in general but I agree with your point. That is why up to this point I hadn't made it an object but in order to make things easier to reason about and maintain going forward, I made it an object :) On Wed, Apr 9, 2014 at 2:36 AM, <arv@chromium.org> wrote: > > https://codereview.chromium.org/223363002/diff/80001/ > chrome/browser/resources/pdf/pdf.js > File chrome/browser/resources/pdf/pdf.js (right): > > https://codereview.chromium.org/223363002/diff/80001/ > chrome/browser/resources/pdf/pdf.js#newcode18 > chrome/browser/resources/pdf/pdf.js:18: this.sizer_ = $('sizer'); > On 2014/04/08 01:44:06, raymes wrote: > >> On 2014/04/07 21:01:44, arv wrote: >> > This pattern is a bit strange. You are creating a constructor which >> > implies > >> that >> > you want to be able to create multiple instances of PDFViewer, yet, >> > you are > >> hard >> > coding to use a single element in the page. If I create 2 PDFViewer >> > instances > >> > there will be conflicts. >> > >> > > It is a bit weird. It would be less weird if the document was passed >> > in as an > >> argument on construction. Do you think it's worth making that change? >> > > I don't think that just because there is a constructor it implies we >> > want to > >> create multiple instances of it, just as singleton objects have >> > constructors. > >> Really the idea is that there should only be one of these things per >> > document. > >> And that is what we get here because the constructor isn't exposed >> > outside of > >> this file. What do you think? >> > > I added a comment suggestion that there should only be one of these >> > per document > >> :) >> > > Now that I look at it. Why is everything private? I think structuring > this as an object just makes the code harder to maintain and you have to > add all those pesky bind calls everywhere. > > I recommend rewriting this without OO. Wrap it in an IIFE to not > introduce globals: > > (function() { > 'use strict'; > > function onPasswordSubmitted(event) { > ... > } > > function updateProgress(progress) { > progressBar.progress = progress; > ... > } > > ... > > var size = $('sizer'); > var progressBar = $('progress-bar'); > ... > > })(); > > https://codereview.chromium.org/223363002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
LGTM If you need it as an object in the future then this is fine.
The CQ bit was checked by raymes@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/raymes@chromium.org/223363002/100001
Message was sent while issue was closed.
Change committed as 262917 |
