Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
830433002:
Navigation to relative fragments does not work correctly for OOP pdf. (Closed)
DescriptionFix for Navigation to relative fragments does not work correctly for OOP pdf case.
When #XXX is already present in the url and same #XXX is href
for navigation on selection href, then we should not add that
already present in the url then don't add this href.
BUG=447888
Committed: https://crrev.com/6313ce26ea190d5da88e4fe6473b693de37305e2
Cr-Commit-Position: refs/heads/master@{#312075}
Patch Set 1 #Patch Set 2 : Improving patch. #Patch Set 3 : Changing code for OOP case. #
Total comments: 4
Patch Set 4 : Addressing Reviewer's comments. #
Total comments: 1
Patch Set 5 : Improving patch. #Patch Set 6 : Changes after taking all nameddest at load time. #
Total comments: 25
Patch Set 7 : Changes as per review comments. #Patch Set 8 : Changes as per review comments. #
Total comments: 37
Patch Set 9 : Changes as per review comments. #Patch Set 10 : changes as per review comments. #Patch Set 11 : Fixing nameddest directory. #
Total comments: 31
Patch Set 12 : Changes as per review comments................. #Patch Set 13 : Changes as per review comments. #Patch Set 14 : Indentation fix. #
Total comments: 26
Patch Set 15 : Changes as per review comments. #Patch Set 16 : changes done for thestig patch. #Patch Set 17 : Changes as per review comments. #
Total comments: 26
Patch Set 18 : Changes as per review comments. #Patch Set 19 : Changes as per review comments. #
Total comments: 5
Patch Set 20 : Changes as per review comments. #Patch Set 21 : Changes as per review comments. #Patch Set 22 : #Patch Set 23 : Rebase code changes. #Patch Set 24 : Fixing Build error. #
Messages
Total messages: 50 (8 generated)
deepak.m1@samsung.com changed reviewers: + raymes@chromium.org, thestig@chromium.org
PTAL at my changes. Thanks
https://codereview.chromium.org/830433002/diff/40001/pdf/out_of_process_insta... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/830433002/diff/40001/pdf/out_of_process_insta... pdf/out_of_process_instance.cc:845: if (url_copy[url_copy.length() - 1] == 0) This looks like a bug in PDFiumPage::GetLinkTarget(). Can we fix that in a separate CL? Does having the trailing '\0' matter? https://codereview.chromium.org/830433002/diff/40001/pdf/out_of_process_insta... pdf/out_of_process_instance.cc:855: if (!open_in_new_tab) { Why are we adding this block? Can't we just navigate normally using the PostMessage() below?
I think part of the problem here is that navigations aren't working at all from inside BrowserPlugin. I will take a look at that.
Changes done and comments updated. PTAL. Thanks https://codereview.chromium.org/830433002/diff/40001/pdf/out_of_process_insta... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/830433002/diff/40001/pdf/out_of_process_insta... pdf/out_of_process_instance.cc:845: if (url_copy[url_copy.length() - 1] == 0) On 2015/01/06 02:13:16, Lei Zhang wrote: > This looks like a bug in PDFiumPage::GetLinkTarget(). Can we fix that in a > separate CL? Does having the trailing '\0' matter? Yes, You are right, This is not required here. https://codereview.chromium.org/830433002/diff/40001/pdf/out_of_process_insta... pdf/out_of_process_instance.cc:855: if (!open_in_new_tab) { On 2015/01/06 02:13:16, Lei Zhang wrote: > Why are we adding this block? Can't we just navigate normally using the > PostMessage() below? I need to add this block here as if I just pass url_copy in PostMessage() then normal navigation is not happening. As Pointed out by raymes also. Due to this, This block of code required to scroll the page to corresponding href.
On 2015/01/06 05:38:22, Deepak wrote: > On 2015/01/06 02:13:16, Lei Zhang wrote: > > Why are we adding this block? Can't we just navigate normally using the > > PostMessage() below? > > I need to add this block here as if I just pass url_copy in PostMessage() then > normal navigation is not happening. > As Pointed out by raymes also. > Due to this, This block of code required to scroll the page to corresponding > href. Did https://codereview.chromium.org/787223005 just fix this problem?
On 2015/01/06 23:16:37, Lei Zhang wrote: > On 2015/01/06 05:38:22, Deepak wrote: > > On 2015/01/06 02:13:16, Lei Zhang wrote: > > > Why are we adding this block? Can't we just navigate normally using the > > > PostMessage() below? > > > > I need to add this block here as if I just pass url_copy in PostMessage() then > > normal navigation is not happening. > > As Pointed out by raymes also. > > Due to this, This block of code required to scroll the page to corresponding > > href. > > Did https://codereview.chromium.org/787223005 just fix this problem? No, https://codereview.chromium.org/787223005 does not fix the issue. With https://codereview.chromium.org/787223005, opening a new link in the window is working fine. But still have below 2 issues. 1) open http://examples.itextpdf.com/results/part1/chapter02/movie_links_1.pdf and go to the last page then select "Go back to first page". Then on first click page in not moving to the first page. 2) open http://examples.itextpdf.com/results/part1/chapter02/movie_links_1.pdf#US here I have put #US in the end knowingly. and go to the last page then select "Go back to first page". Then on first click page in not moving to the first page. With my patch both these things getting fixed.
On 2015/01/07 03:53:13, Deepak wrote: > 2) open > http://examples.itextpdf.com/results/part1/chapter02/movie_links_1.pdf#US > here I have put #US in the end knowingly. > and go to the last page then select "Go back to first page". > Then on first click page in not moving to the first page. pdf/instance.cc aka In Process PDF has the #US#US problem too. Can we fix that here too?
https://codereview.chromium.org/830433002/diff/60001/pdf/out_of_process_insta... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/830433002/diff/60001/pdf/out_of_process_insta... pdf/out_of_process_instance.cc:854: found_idx = url_copy.find('#'); There's some weird problems with OOP PDF. Without this if-block, open a new tab, go to http://examples.itextpdf.com/results/part1/chapter02/movie_links_1.pdf, scroll to the last page, and click the #US link -> results in no navigation. Clicking it a second time works. The other issue is when it does work, we are not simply scrolling to the page with the anchor. Instead, we are reloading the document. We shouldn't have to reload. This may be out of the scope of this CL, but I want to mention it. In-Process PDF doesn't have these problems, but it does suffer from the #US#US issue.
I'm still digging into the navigation issues in OOP PDF, it's not fully fixed. On Thu Jan 08 2015 at 10:08:45 AM <thestig@chromium.org> wrote: > > https://codereview.chromium.org/830433002/diff/60001/pdf/ > out_of_process_instance.cc > File pdf/out_of_process_instance.cc (right): > > https://codereview.chromium.org/830433002/diff/60001/pdf/ > out_of_process_instance.cc#newcode854 > pdf/out_of_process_instance.cc:854 > <https://codereview.chromium.org/830433002/diff/60001/pdf/out_of_process_insta...>: > found_idx = url_copy.find('#'); > There's some weird problems with OOP PDF. Without this if-block, open a > new tab, go to > http://examples.itextpdf.com/results/part1/chapter02/movie_links_1.pdf, > scroll to the last page, and click the #US link -> results in no > navigation. Clicking it a second time works. > > The other issue is when it does work, we are not simply scrolling to the > page with the anchor. Instead, we are reloading the document. We > shouldn't have to reload. This may be out of the scope of this CL, but I > want to mention it. > > In-Process PDF doesn't have these problems, but it does suffer from the > #US#US issue. > > https://codereview.chromium.org/830433002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I had a closer look at this. thestig: problem is that when I moved some of the functionality into the javascript side, I didn't preserve all the behavior. We used to use the GetInitialPage function here but we've removed that and moved the functionality into JS. The named destination part of that functionality isn't implemented yet that's on my plate (see https://code.google.com/p/chromium/issues/detail?id=416339). Deepak - can we move this whole function into the JS side? We should reuse the OpenPDFParamsParser to pull out the page to navigate to.
On 2015/01/07 23:08:43, Lei Zhang wrote: > https://codereview.chromium.org/830433002/diff/60001/pdf/out_of_process_insta... > File pdf/out_of_process_instance.cc (right): > > https://codereview.chromium.org/830433002/diff/60001/pdf/out_of_process_insta... > pdf/out_of_process_instance.cc:854: found_idx = url_copy.find('#'); > There's some weird problems with OOP PDF. Without this if-block, open a new tab, > go to http://examples.itextpdf.com/results/part1/chapter02/movie_links_1.pdf, > scroll to the last page, and click the #US link -> results in no navigation. > Clicking it a second time works. > > The other issue is when it does work, we are not simply scrolling to the page > with the anchor. Instead, we are reloading the document. We shouldn't have to > reload. This may be out of the scope of this CL, but I want to mention it. > > In-Process PDF doesn't have these problems, but it does suffer from the #US#US > issue. I will submit patch for in process case,today itself.
can you please suggest way to get pagenumber from '#US' from open_pdf_params_parser.js file,as we can get page number by engine_->GetNamedDestinationPage(). and How about passing page number from out_of_instance.cc file to pdf.js and then doing scrolling their itself. This will give smooth effect, I have tried and working as expected, changes uploaded. PTAL
open_pdf_params_parser.js is supposed to know about named destinations as well, that hasn't been implemented yet (see https://code.google.com/p/ chromium/issues/detail?id=416339). These should be fixed as a few separate issues: 1) Make open_pdf_params_parser.js know about named destinations by sending all of the named destinations to JS on document load. This should fix the currently outstanding bug where we don't navigate to named destinations properly when they are contained in the URL upon load. 2) When trying to do a navigation, send the URL to JS and do all the current checks in the JS side. We can reuse open_pdf_params_parser.js to determine if there is a page # or named destination to navigate to. On Thu Jan 08 2015 at 6:21:02 PM <deepak.m1@samsung.com> wrote: > > can you please suggest way to get pagenumber from '#US' from > open_pdf_params_parser.js file,as we can get > page number by engine_->GetNamedDestinationPage(). > > and How about passing page number from out_of_instance.cc file to pdf.js > and > then doing scrolling their itself. > This will give smooth effect, > I have tried and working as expected, changes uploaded. > PTAL > > https://codereview.chromium.org/830433002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/01/08 22:04:22, raymes wrote: > open_pdf_params_parser.js is supposed to know about named destinations as > well, that hasn't been implemented yet (see https://code.google.com/p/ > chromium/issues/detail?id=416339). These should be fixed as a few separate > issues: > > 1) Make open_pdf_params_parser.js know about named destinations by sending > all of the named destinations to JS on document load. This should fix the > currently outstanding bug where we don't navigate to named destinations > properly when they are contained in the URL upon load. > 2) When trying to do a navigation, send the URL to JS and do all the > current checks in the JS side. We can reuse open_pdf_params_parser.js to > determine if there is a page # or named destination to navigate to. > > On Thu Jan 08 2015 at 6:21:02 PM <mailto:deepak.m1@samsung.com> wrote: > > > > > can you please suggest way to get pagenumber from '#US' from > > open_pdf_params_parser.js file,as we can get > > page number by engine_->GetNamedDestinationPage(). > > > > and How about passing page number from out_of_instance.cc file to pdf.js > > and > > then doing scrolling their itself. > > This will give smooth effect, > > I have tried and working as expected, changes uploaded. > > PTAL > > > > https://codereview.chromium.org/830433002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. ok, I will start working in this issue. Thanks
I have checked the implementation of https://codereview.chromium.org/830433002/. We need all the nameDests and their corresponding page numbers for internal scroll. For example, in this issue, I whould all the #XXX tags from FPDF_GetNamedDest() api, so that I will have corresponding page number. At the time of loading the page.and we can handle nameDest at the JS side. And key value pairs for other cases. But I am not getting these properly from FPDF_GetNamedDest(). I am getting single Alphabat for name, and that also comes menay times and it have different page number. I suspect that I am getting handle of nameDest instead of name. I request @ Bo Xu to please check this. I am using below in a loop to get all names like: -- char* name = NULL; unsigned int len = 0; FPDF_DEST dest = NULL; dest = FPDF_GetNamedDest(doc_,i,name,len); if (dest) { name = new char[len]; FPDF_DEST dest = FPDF_GetNamedDest(doc_,i,name,len); if (dest) { LOG(INFO)<<"Deepak :"<<i<<" page:"<<name<<","<<len<<","<<FPDFDest_GetPageIndex(doc_, dest); } delete []name; } -- Please correct me if my understanding is wrong.
deepak.m1@samsung.com changed reviewers: + bo_xu@chromium.org
deepak.m1@samsung.com changed reviewers: + gene@chromium.org
@raymes & @gene I have made changes based on https://codereview.chromium.org/834703002/. so my changes should go after above review changes rolled in. Now we are getting all the nameddests of document at the load time only. and we have this info in the .js that we will use when use select some link. link #US in our issue case. With these changes issue get fixed. PTAL.
On 2015/01/11 09:01:17, Deepak wrote: > @raymes & @gene > I have made changes based on https://codereview.chromium.org/834703002/. > so my changes should go after above review changes rolled in. > > Now we are getting all the nameddests of document at the load time only. > and we have this info in the .js that we will use when use select some link. > link #US in our issue case. > With these changes issue get fixed. > PTAL. https://codereview.chromium.org/834703002/ changes already merged. so now no dependency. PTAL and share your thoughts.
@raymes & @gene I have one query: currently getting nameddest from the url is happening in parseOpenPDFParams_() in open_pdf_params_parser.js, like for nameddest,zoom,page etc. Is their any plan to do this alone with the PDF document load as we are getting nameddest for document at the load time, We can get nameddest,zoom,page etc info from the url at PDF load time. or we can keep them separated like URL param parsing in js and other nameddest from doc at the PDF load time. Just wanted to know your opinion for above.
Thanks Deepak! This needs a bunch of reworking but is on the right track. I'm not exactly sure what your question is but if it's not answered in my comments below, please let me know. Raymes https://codereview.chromium.org/830433002/diff/100001/chrome/browser/resource... File chrome/browser/resources/pdf/open_pdf_params_parser.js (right): https://codereview.chromium.org/830433002/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:66: this.urlParams['nameddest'] = paramTokens[0]; We don't actually want to store nameddest in urlParams. Instead, we want to use it to determine urlParams['page'] based on the list of named destinations which get sent from the plugin. So instead do something like var namedDest; ... namedDest = paramTokens[0]; and then down the bottom something like: if ('nameddest' in paramsDictionary) namedDest = paramsDictionary['nameddest']; and then if (namedDest) this.urlParams['page'] = namedDestinations_[namedDest]; if ( https://codereview.chromium.org/830433002/diff/100001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/830433002/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:140: this.urlParams_ = paramsParser.urlParams; Let's move these two lines of code into handleURLParams_. That will allow the params to be set up after the named destinations have been received. It will also allow us to remove the urlParams_ instance variable and replace it with a local variable. https://codereview.chromium.org/830433002/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:142: this.namedDestinations_ = {}; Rather than storing these here, maybe we can just store them in the pdf params parser for now. https://codereview.chromium.org/830433002/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:385: this.namedDestinations_ = message.data.namedDestinations; Let's add a new message type for named destinations. It should always be sent before document load is complete. https://codereview.chromium.org/830433002/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:398: chrome.tabs.update(this.streamDetails_.tabId, We don't need to use the chrome.tabs API here - the window.open/window.location method should be working now on ToT chrome. https://codereview.chromium.org/830433002/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:401: } Let's move all of this code into a new function. The function should be similar to Instance::NavigateTo in instance.cc (it should do the same things). Instead of calling GetInitialPage() (as the cc file does) we should call into the open pdf params parser which should have equivalent functionality. https://codereview.chromium.org/830433002/diff/100001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/830433002/diff/100001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:880: } Can we move all of the above code into a function in the JS side? https://codereview.chromium.org/830433002/diff/100001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:1333: void OutOfProcessInstance::GetNamedDestinations( Can we move all of this inside pdfium_engine and just have a function there that returns a pp::VarDictionary? https://codereview.chromium.org/830433002/diff/100001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:1334: pp::VarDictionary& named_destinations) { for future reference: we never use references that are out-params https://codereview.chromium.org/830433002/diff/100001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:1359: } Is all of this really needed?? Does FPDF_GetNamedDest really ever return something that looks like nameddest=US? That seems really weird to me. This code looks like it was used to get the nameddest from the URL but I suspect we don't need it here. https://codereview.chromium.org/830433002/diff/100001/pdf/pdf_engine.h File pdf/pdf_engine.h (right): https://codereview.chromium.org/830433002/diff/100001/pdf/pdf_engine.h#newcod... pdf/pdf_engine.h:223: virtual int GetNamedDestinationPage(const std::string& destination) = 0; We can probably remove this function now
Thanks for review and sharing your thoughts. Changes done as suggested. PTAL at my changes. Thanks. https://codereview.chromium.org/830433002/diff/100001/chrome/browser/resource... File chrome/browser/resources/pdf/open_pdf_params_parser.js (right): https://codereview.chromium.org/830433002/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:66: this.urlParams['nameddest'] = paramTokens[0]; On 2015/01/11 23:08:41, raymes wrote: > We don't actually want to store nameddest in urlParams. Instead, we want to use > it to determine urlParams['page'] based on the list of named destinations which > get sent from the plugin. So instead do something like > > var namedDest; > ... > namedDest = paramTokens[0]; > > and then down the bottom something like: > if ('nameddest' in paramsDictionary) > namedDest = paramsDictionary['nameddest']; > > and then > if (namedDest) > this.urlParams['page'] = namedDestinations_[namedDest]; > > if ( Done. https://codereview.chromium.org/830433002/diff/100001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/830433002/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:140: this.urlParams_ = paramsParser.urlParams; On 2015/01/11 23:08:41, raymes wrote: > Let's move these two lines of code into handleURLParams_. That will allow the > params to be set up after the named destinations have been received. It will > also allow us to remove the urlParams_ instance variable and replace it with a > local variable. I agree, we don't need these 2 variables here. https://codereview.chromium.org/830433002/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:142: this.namedDestinations_ = {}; On 2015/01/11 23:08:41, raymes wrote: > Rather than storing these here, maybe we can just store them in the pdf params > parser for now. Done. https://codereview.chromium.org/830433002/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:385: this.namedDestinations_ = message.data.namedDestinations; On 2015/01/11 23:08:41, raymes wrote: > Let's add a new message type for named destinations. It should always be sent > before document load is complete. Done. I have added a message and sending that when DocumentLoadComplete() just started. https://codereview.chromium.org/830433002/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:398: chrome.tabs.update(this.streamDetails_.tabId, On 2015/01/11 23:08:41, raymes wrote: > We don't need to use the chrome.tabs API here - the window.open/window.location > method should be working now on ToT chrome. window.location.href = message.data.url; is OK, But using window.open(message.data.url); is causing crash, so I need to use chrome.tabs.create({ url: message.data.url }); thing here. https://codereview.chromium.org/830433002/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:401: } On 2015/01/11 23:08:41, raymes wrote: > Let's move all of this code into a new function. The function should be similar > to Instance::NavigateTo in instance.cc (it should do the same things). Instead > of calling GetInitialPage() (as the cc file does) we should call into the open > pdf params parser which should have equivalent functionality. Done. https://codereview.chromium.org/830433002/diff/100001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/830433002/diff/100001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:880: } On 2015/01/11 23:08:41, raymes wrote: > Can we move all of the above code into a function in the JS side? Done. https://codereview.chromium.org/830433002/diff/100001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:1333: void OutOfProcessInstance::GetNamedDestinations( On 2015/01/11 23:08:41, raymes wrote: > Can we move all of this inside pdfium_engine and just have a function there that > returns a pp::VarDictionary? Done. https://codereview.chromium.org/830433002/diff/100001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:1334: pp::VarDictionary& named_destinations) { On 2015/01/11 23:08:41, raymes wrote: > for future reference: we never use references that are out-params Done. https://codereview.chromium.org/830433002/diff/100001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:1359: } On 2015/01/11 23:08:41, raymes wrote: > Is all of this really needed?? Does FPDF_GetNamedDest really ever return > something that looks like nameddest=US? That seems really weird to me. This code > looks like it was used to get the nameddest from the URL but I suspect we don't > need it here. I agree with you, This code currently no needed,we get the namedests like US UK AR PK and then using these I found out the corresponding page number in the PDF , So removing this code. https://codereview.chromium.org/830433002/diff/100001/pdf/pdf_engine.h File pdf/pdf_engine.h (right): https://codereview.chromium.org/830433002/diff/100001/pdf/pdf_engine.h#newcod... pdf/pdf_engine.h:223: virtual int GetNamedDestinationPage(const std::string& destination) = 0; On 2015/01/11 23:08:41, raymes wrote: > We can probably remove this function now This function is currently getting used in instance.cc file. Once instance.cc file code get removed we can remove this function. Till that time we need to keep this.
Thanks Deepak, this is getting closer :) We will also need a test for some of this stuff (we can do this in a separate CL). Please look at pdf_extension_test.cc to see how it runs tests and we can discuss a bit more how we should write the tests after this CL. https://codereview.chromium.org/830433002/diff/100001/chrome/browser/resource... File chrome/browser/resources/pdf/open_pdf_params_parser.js (right): https://codereview.chromium.org/830433002/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:64: // mentioned except by example in the Adobe "PDF Open Parameters" document. How come you removed this? https://codereview.chromium.org/830433002/diff/100001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:66: this.urlParams['nameddest'] = paramTokens[0]; Please take a look at the comment above again, in particular the last 2 lines of code. We want to have this right at the bottom of this function if (namedDest) this.urlParams['page'] = namedDestinations_[namedDest]; https://codereview.chromium.org/830433002/diff/100001/pdf/pdf_engine.h File pdf/pdf_engine.h (right): https://codereview.chromium.org/830433002/diff/100001/pdf/pdf_engine.h#newcod... pdf/pdf_engine.h:223: virtual int GetNamedDestinationPage(const std::string& destination) = 0; Ah right, thanks :) https://codereview.chromium.org/830433002/diff/140001/chrome/browser/resource... File chrome/browser/resources/pdf/open_pdf_params_parser.js (right): https://codereview.chromium.org/830433002/diff/140001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:14: this.namedDest; I don't think you need to store this as an instance variable https://codereview.chromium.org/830433002/diff/140001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/830433002/diff/140001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:347: * Helper function to process url from message and to navigate with in the nit: "with in" -> "within" https://codereview.chromium.org/830433002/diff/140001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:380: if (this.paramsParser.namedDestinations[urlStr] != undefined) Rather than just getting the named destination out of the params parser, we want to re-use the logic in the params parser to get the page that should be navigated to. The reason is that the URL might contain something like #page=N which we should also support properly here. We can modify the params parser to support this by changing the constructor of it to no longer take a url, so you would just construct a params parser: new OpenPDFParamsParser(); Then you would call a function to get the URL params for a given URL: this.paramsParser.getURLParams(url) Then we can use the page number that it returns as the page to navigate to (this corresponds to the GetInitialPage function inside instance.cc). This way we could reuse the paramsParser here and inside handleUrlParams https://codereview.chromium.org/830433002/diff/140001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:385: } I think the logic in this function needs some work. Let's try to follow the logic in instance.cc closely unless there is a good reason not to. This will avoid introducing bugs. https://codereview.chromium.org/830433002/diff/140001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:432: this.navigate_(message.data); can we pull out the url and newTab properties and pass them separately to the function? https://codereview.chromium.org/830433002/diff/140001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/830433002/diff/140001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:856: } Can we move this code into JS as well? We should have the original URL available there too (this.streamDetails_.originalUrl) https://codereview.chromium.org/830433002/diff/140001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:1071: pp::VarDictionary named_message; nit: move this down to right above where it is used. Maybe call it named_destinations_message https://codereview.chromium.org/830433002/diff/140001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:1073: engine_->GetAllNameDests(&named_destinations); Can't this function just return a pp::VarDictionary rather than having an out parameter? https://codereview.chromium.org/830433002/diff/140001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:1076: PostMessage(named_message); nit: let's move the above code down to right above the progress message https://codereview.chromium.org/830433002/diff/140001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:1092: pp::VarDictionary message; Maybe call this progress_message now since there are 2 different messages https://codereview.chromium.org/830433002/diff/140001/pdf/pdf_engine.h File pdf/pdf_engine.h (right): https://codereview.chromium.org/830433002/diff/140001/pdf/pdf_engine.h#newcod... pdf/pdf_engine.h:267: virtual void GetAllNameDests(pp::VarDictionary* named_destinations) = 0; nit: Let's make it verbose: GetAllNamedDestinations https://codereview.chromium.org/830433002/diff/140001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/830433002/diff/140001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:97: const char kDelimiters[] = "#&"; We can remove this (see below) https://codereview.chromium.org/830433002/diff/140001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1196: return FPDF_CountNamedDests(doc_); This is quite simple, so let's inline it :) https://codereview.chromium.org/830433002/diff/140001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1199: void PDFiumEngine::GetNameDests(std::vector<std::string>* name_dest) { Given the simplifications I mentioned below, I think we should be able to merge this function with the one below and probably simplify things a bit https://codereview.chromium.org/830433002/diff/140001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1202: wchar_t* name = NULL; I think we should use a std::string16 and the WriteInto function (look for this pattern elsewhere in this file, e.g. in line 3800) https://codereview.chromium.org/830433002/diff/140001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1206: if (dest) { -This is strange - the API documentation says it should return NULL if we pass in a NULL name. In any case, we shouldn't check the return value here, there is no need to. We can remove the above dest variable altogether. https://codereview.chromium.org/830433002/diff/140001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1225: (fragments[0].find('=') == std::string::npos)) { I don't think we need the above 3 lines. I suspect the name of the named destination will always satisfy these constraints. It would be weird for the API to return something else. https://codereview.chromium.org/830433002/diff/140001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1226: int page_number = GetNamedDestinationPage(fragments[0]); We can just have GetNamedDestinationPage(name_dest[i])
PTAL. Thanks https://codereview.chromium.org/830433002/diff/140001/chrome/browser/resource... File chrome/browser/resources/pdf/open_pdf_params_parser.js (right): https://codereview.chromium.org/830433002/diff/140001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:14: this.namedDest; On 2015/01/13 01:59:31, raymes wrote: > I don't think you need to store this as an instance variable agreed https://codereview.chromium.org/830433002/diff/140001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/830433002/diff/140001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:347: * Helper function to process url from message and to navigate with in the On 2015/01/13 01:59:31, raymes wrote: > nit: "with in" -> "within" Done. https://codereview.chromium.org/830433002/diff/140001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:380: if (this.paramsParser.namedDestinations[urlStr] != undefined) On 2015/01/13 01:59:31, raymes wrote: > Rather than just getting the named destination out of the params parser, we want > to re-use the logic in the params parser to get the page that should be > navigated to. The reason is that the URL might contain something like #page=N > which we should also support properly here. > > We can modify the params parser to support this by changing the constructor of > it to no longer take a url, so you would just construct a params parser: > new OpenPDFParamsParser(); > > Then you would call a function to get the URL params for a given URL: > this.paramsParser.getURLParams(url) > > Then we can use the page number that it returns as the page to navigate to (this > corresponds to the GetInitialPage function inside instance.cc). > > This way we could reuse the paramsParser here and inside handleUrlParams Done. https://codereview.chromium.org/830433002/diff/140001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:385: } On 2015/01/13 01:59:31, raymes wrote: > I think the logic in this function needs some work. Let's try to follow the > logic in instance.cc closely unless there is a good reason not to. This will > avoid introducing bugs. > I have added a new function on lines of getInitalPage(), that gives us pageNumber for the '#page=X' case and '#namedest=x' or '#namedest' case. so now we don't need nameDest variable. and code is also simplified. https://codereview.chromium.org/830433002/diff/140001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:432: this.navigate_(message.data); On 2015/01/13 01:59:31, raymes wrote: > can we pull out the url and newTab properties and pass them separately to the > function? Done. https://codereview.chromium.org/830433002/diff/140001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/830433002/diff/140001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:856: } On 2015/01/13 01:59:31, raymes wrote: > Can we move this code into JS as well? We should have the original URL available > there too (this.streamDetails_.originalUrl) Done. https://codereview.chromium.org/830433002/diff/140001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:1071: pp::VarDictionary named_message; On 2015/01/13 01:59:31, raymes wrote: > nit: move this down to right above where it is used. Maybe call it > named_destinations_message Done. https://codereview.chromium.org/830433002/diff/140001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:1073: engine_->GetAllNameDests(&named_destinations); On 2015/01/13 01:59:31, raymes wrote: > Can't this function just return a pp::VarDictionary rather than having an out > parameter? Done. https://codereview.chromium.org/830433002/diff/140001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:1076: PostMessage(named_message); On 2015/01/13 01:59:31, raymes wrote: > nit: let's move the above code down to right above the progress message Done. https://codereview.chromium.org/830433002/diff/140001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:1092: pp::VarDictionary message; On 2015/01/13 01:59:31, raymes wrote: > Maybe call this progress_message now since there are 2 different messages Done. https://codereview.chromium.org/830433002/diff/140001/pdf/pdf_engine.h File pdf/pdf_engine.h (right): https://codereview.chromium.org/830433002/diff/140001/pdf/pdf_engine.h#newcod... pdf/pdf_engine.h:267: virtual void GetAllNameDests(pp::VarDictionary* named_destinations) = 0; On 2015/01/13 01:59:31, raymes wrote: > nit: Let's make it verbose: GetAllNamedDestinations Done. https://codereview.chromium.org/830433002/diff/140001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/830433002/diff/140001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:97: const char kDelimiters[] = "#&"; On 2015/01/13 01:59:32, raymes wrote: > We can remove this (see below) Done. https://codereview.chromium.org/830433002/diff/140001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1196: return FPDF_CountNamedDests(doc_); On 2015/01/13 01:59:32, raymes wrote: > This is quite simple, so let's inline it :) Done. https://codereview.chromium.org/830433002/diff/140001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1196: return FPDF_CountNamedDests(doc_); On 2015/01/13 01:59:32, raymes wrote: > This is quite simple, so let's inline it :) Done. https://codereview.chromium.org/830433002/diff/140001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1199: void PDFiumEngine::GetNameDests(std::vector<std::string>* name_dest) { On 2015/01/13 01:59:32, raymes wrote: > Given the simplifications I mentioned below, I think we should be able to merge > this function with the one below and probably simplify things a bit Done. https://codereview.chromium.org/830433002/diff/140001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1202: wchar_t* name = NULL; On 2015/01/13 01:59:31, raymes wrote: > I think we should use a std::string16 and the WriteInto function (look for this > pattern elsewhere in this file, e.g. in line 3800) done https://codereview.chromium.org/830433002/diff/140001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1206: if (dest) { On 2015/01/13 01:59:32, raymes wrote: > -This is strange - the API documentation says it should return NULL if we pass > in a NULL name. In any case, we shouldn't check the return value here, there is > no need to. We can remove the above dest variable altogether. agreed https://codereview.chromium.org/830433002/diff/140001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1225: (fragments[0].find('=') == std::string::npos)) { On 2015/01/13 01:59:32, raymes wrote: > I don't think we need the above 3 lines. I suspect the name of the named > destination will always satisfy these constraints. It would be weird for the API > to return something else. Done. https://codereview.chromium.org/830433002/diff/140001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1226: int page_number = GetNamedDestinationPage(fragments[0]); On 2015/01/13 01:59:32, raymes wrote: > We can just have GetNamedDestinationPage(name_dest[i]) Done.
Thanks! https://codereview.chromium.org/830433002/diff/200001/chrome/browser/resource... File chrome/browser/resources/pdf/open_pdf_params_parser.js (right): https://codereview.chromium.org/830433002/diff/200001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:14: // This will have name of all the nameddest from the loaded PDF. nit: // A dictionary of all the named destinations in the PDF. https://codereview.chromium.org/830433002/diff/200001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:116: }, I don't think we need to add a new function to parse the URL. We already parse the URL in getURLPDFParams. We just need to add the part the looks up the nameddest if it exists and alter the page number, as I mentioned earlier. Maybe you're concerned that we're no longer really returning "url params" anymore. That's ok we should just change the name of the function to "getInitialViewport" (or something similar) https://codereview.chromium.org/830433002/diff/200001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/830433002/diff/200001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:348: * PDF page or opening new url in the same tab. nit: Helper function to navigate to the given URL. This might involve navgating within the PDF page or opening a new url (in the same tab or a new tab) https://codereview.chromium.org/830433002/diff/200001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:349: * @param {Object} message data that have the url and newTab info. Please update this with the new params :) https://codereview.chromium.org/830433002/diff/200001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:352: if (urlFragment.length != 0) { nit: everything from here below should be indented one more level https://codereview.chromium.org/830433002/diff/200001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:358: var hashIndex = this.streamDetails_.originalUrl.search('#'); nit: let's put the original url in a local variable above to simplify the code a bit var originalUrl = this.streamDetails_.originalUrl; https://codereview.chromium.org/830433002/diff/200001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:366: } the logic from here down differs from Instance::NavigateTo. Can we keep it similar? In navigateTo it does the internal scrolling at this point https://codereview.chromium.org/830433002/diff/200001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:391: chrome.tabs.create({ url: inputURL }); As I mentioned earlier - we don't need to use the chrome.tabs API. window.open is good enough. https://codereview.chromium.org/830433002/diff/200001/pdf/pdf_engine.h File pdf/pdf_engine.h (right): https://codereview.chromium.org/830433002/diff/200001/pdf/pdf_engine.h#newcod... pdf/pdf_engine.h:265: virtual unsigned long GetNameDestCount() = 0; We don't need this anymore https://codereview.chromium.org/830433002/diff/200001/pdf/pdf_engine.h#newcod... pdf/pdf_engine.h:266: virtual pp::VarDictionary GetAllNamedDestinations() = 0; nit: let's just call it GetNamedDestinations https://codereview.chromium.org/830433002/diff/200001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/830433002/diff/200001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1192: pp::VarDictionary PDFiumEngine::GetAllNamedDestinations() { nit: Can we move this function down near GetNamedDestinationPage? https://codereview.chromium.org/830433002/diff/200001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1194: for (unsigned long i = 0; i < GetNameDestCount(); i++) { Sorry when I said "inline" I just meant actually copying the code into here. It's not worth having a separate function to get the count https://codereview.chromium.org/830433002/diff/200001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1196: size_t buffer_bytes; use unsigned long here (as in the API) https://codereview.chromium.org/830433002/diff/200001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1198: if (buffer_bytes > 1) { We should compute the length of the string here because WriteInto takes the length in chars and not the size of the buffer. The length includes the null character. Divide buffer_length by sizeof(base::string16::value_type) to get name_length if (name_length > 1) ... ... WriteInto(..., name_length) https://codereview.chromium.org/830433002/diff/200001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/830433002/diff/200001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.h:110: } We can remove this
window.open(url); usage is crashing, and I found issues about it is not working properly in chrome are logged. please suggest if their is some way to use this to open url in newTab. Rest changes done. PTAL. https://codereview.chromium.org/830433002/diff/200001/chrome/browser/resource... File chrome/browser/resources/pdf/open_pdf_params_parser.js (right): https://codereview.chromium.org/830433002/diff/200001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:14: // This will have name of all the nameddest from the loaded PDF. On 2015/01/14 00:19:00, raymes wrote: > nit: // A dictionary of all the named destinations in the PDF. Done. https://codereview.chromium.org/830433002/diff/200001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:116: }, On 2015/01/14 00:19:00, raymes wrote: > I don't think we need to add a new function to parse the URL. We already parse > the URL in getURLPDFParams. We just need to add the part the looks up the > nameddest if it exists and alter the page number, as I mentioned earlier. > > Maybe you're concerned that we're no longer really returning "url params" > anymore. That's ok we should just change the name of the function to > "getInitialViewport" (or something similar) I understood your meaning, I am removing getInitialPage() and we get required info from getURLPDFParams(), that already have input url parsed info. and in navigate_() in pdf.js we can get the page number for the tag like #US in our case, as we already have this.paramsParser.namedDestinations{} ready here,so we can completely avoid, parsing url to get the page number for navigate_. only we need to prepare nameddest from urlFragment to pass it to this.paramsParser.namedDestinations[]. Thanks for suggestion and guidance. I have made the changes. Thanks https://codereview.chromium.org/830433002/diff/200001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/830433002/diff/200001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:348: * PDF page or opening new url in the same tab. On 2015/01/14 00:19:00, raymes wrote: > nit: Helper function to navigate to the given URL. This might involve navgating > within the PDF page or opening a new url (in the same tab or a new tab) Done. https://codereview.chromium.org/830433002/diff/200001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:349: * @param {Object} message data that have the url and newTab info. On 2015/01/14 00:19:00, raymes wrote: > Please update this with the new params :) Done. https://codereview.chromium.org/830433002/diff/200001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:352: if (urlFragment.length != 0) { On 2015/01/14 00:19:00, raymes wrote: > nit: everything from here below should be indented one more level Done. https://codereview.chromium.org/830433002/diff/200001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:358: var hashIndex = this.streamDetails_.originalUrl.search('#'); On 2015/01/14 00:19:00, raymes wrote: > nit: let's put the original url in a local variable above to simplify the code a > bit > > var originalUrl = this.streamDetails_.originalUrl; Done. https://codereview.chromium.org/830433002/diff/200001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:366: } On 2015/01/14 00:19:00, raymes wrote: > the logic from here down differs from Instance::NavigateTo. Can we keep it > similar? In navigateTo it does the internal scrolling at this point If I scroll here like Instance::NavigateTo() then we are getting jurky effect in the page, and it will not be smooth scrolling in the page.and I am getting effect like page is getting loaded again. As thestig mentioned that we want smooth scrolling when navigation with in the page. https://codereview.chromium.org/830433002/diff/200001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:391: chrome.tabs.create({ url: inputURL }); On 2015/01/14 00:19:00, raymes wrote: > As I mentioned earlier - we don't need to use the chrome.tabs API. window.open > is good enough. I remember, you told me to use window.open(url); But this call is crashing. I found that window.open does not work properly issue already logged. I tried window.open(inputURl,'_self') or window.open(inputURl,'_parent') but these open the link in the same tab. https://codereview.chromium.org/830433002/diff/200001/pdf/pdf_engine.h File pdf/pdf_engine.h (right): https://codereview.chromium.org/830433002/diff/200001/pdf/pdf_engine.h#newcod... pdf/pdf_engine.h:265: virtual unsigned long GetNameDestCount() = 0; On 2015/01/14 00:19:00, raymes wrote: > We don't need this anymore Done. https://codereview.chromium.org/830433002/diff/200001/pdf/pdf_engine.h#newcod... pdf/pdf_engine.h:266: virtual pp::VarDictionary GetAllNamedDestinations() = 0; On 2015/01/14 00:19:00, raymes wrote: > nit: let's just call it GetNamedDestinations Done. https://codereview.chromium.org/830433002/diff/200001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/830433002/diff/200001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1192: pp::VarDictionary PDFiumEngine::GetAllNamedDestinations() { On 2015/01/14 00:19:01, raymes wrote: > nit: Can we move this function down near GetNamedDestinationPage? Done. https://codereview.chromium.org/830433002/diff/200001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1194: for (unsigned long i = 0; i < GetNameDestCount(); i++) { On 2015/01/14 00:19:01, raymes wrote: > Sorry when I said "inline" I just meant actually copying the code into here. > It's not worth having a separate function to get the count Done. https://codereview.chromium.org/830433002/diff/200001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1196: size_t buffer_bytes; On 2015/01/14 00:19:01, raymes wrote: > use unsigned long here (as in the API) Done. https://codereview.chromium.org/830433002/diff/200001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1198: if (buffer_bytes > 1) { On 2015/01/14 00:19:01, raymes wrote: > We should compute the length of the string here because WriteInto takes the > length in chars and not the size of the buffer. The length includes the null > character. Divide buffer_length by sizeof(base::string16::value_type) to get > name_length > > if (name_length > 1) > ... > ... WriteInto(..., name_length) Done. https://codereview.chromium.org/830433002/diff/200001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/830433002/diff/200001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.h:110: } On 2015/01/14 00:19:01, raymes wrote: > We can remove this Done.
The crash is happening due to :
DCHECK(new_contents); failure in
WebContentsImpl* BrowserPluginGuest::CreateNewGuestWindow() as
WebContentsImpl* new_contents =
static_cast<WebContentsImpl*>(delegate_->CreateNewGuestWindow(params));
return NULL.
as below function returns NULL.
WebContents* BrowserPluginGuestDelegate::CreateNewGuestWindow(
const WebContents::CreateParams& create_params) {
return NULL;
}
the call stack is:
#0 0x7f40fa78f20e base::debug::StackTrace::StackTrace()
#1 0x7f40fa808115 logging::LogMessage::~LogMessage()
#2 0x7f40ff8dc1be content::BrowserPluginGuest::CreateNewGuestWindow()
#3 0x7f40ff70e634 content::WebContentsImpl::CreateNewWindow()
#4 0x7f40ff70eb1e content::WebContentsImpl::CreateNewWindow()
#5 0x7f40ff56e595 content::RenderViewHostImpl::CreateNewWindow()
#6 0x7f40ff577b55 content::RenderWidgetHelper::OnCreateWindowOnUI()
#7 0x7f40ff579899 base::internal::RunnableAdapter<>::Run()
#8 0x7f40ff5797a4 base::internal::InvokeHelper<>::MakeItSo()
so till this crash get fixed, we wil use
chrome.tabs.create({ url: inputURL }); for opening url in the newTab.
Thanks
Thanks! https://codereview.chromium.org/830433002/diff/200001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/830433002/diff/200001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:366: } What I mean is to move the following lines below up to here: if (namedFragment) pageNumber = this.paramsParser.namedDestinations[namedFragment]; Does moving those up into the same place as inside NavigateTo cause something to become janky? https://codereview.chromium.org/830433002/diff/260001/chrome/browser/resource... File chrome/browser/resources/pdf/open_pdf_params_parser.js (right): https://codereview.chromium.org/830433002/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:58: getURLPDFParams: function(inputUrl) { nit: let's change the name because I think it is doing something different. getViewportFromUrlParams https://codereview.chromium.org/830433002/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:68: } I don't think we need this whole if-block https://codereview.chromium.org/830433002/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:81: this.urlParams['nameddest'] = pageNumber; Can we just store this in the 'page' param as well? https://codereview.chromium.org/830433002/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:87: if (!isNaN(pageNumber) && pageNumber > 0) good catch! thanks :) https://codereview.chromium.org/830433002/diff/260001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/830433002/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:139: this.paramsParser = new OpenPDFParamsParser(); Let's keep this named paramsParser_. An underscore at the end means it's private to the class https://codereview.chromium.org/830433002/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:291: this.viewport_.goToPage(urlParams.nameddest); If we merge nameddest and page we can remove this if-statement https://codereview.chromium.org/830433002/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:353: navigate_: function(urlFragment, isNewTab) { I'd like to start reducing this size of this class so can we move this function into a separate class/file? We can call it URLNavigator or something and it would just have a single function which is navigate. https://codereview.chromium.org/830433002/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:393: chrome.tabs.create({ url: inputURL }); Cool, thanks for looking into that! Let's leave it as you have it for now then :) https://codereview.chromium.org/830433002/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:403: }, Here is what I was thinking (there are a few changes from what you have). I've stripped out all the comments but please add them back in! :) navigate_: function(url, isNewTab) { if (url.length == 0) return; var originalUrl = this.streamDetails_.originalUrl; if (url[0] == '#') { var hashIndex = originalUrl.search('#') if (hashIndex != -1) url = originalUrl.substring(0, hashIndex) + url else url = originalUrl + url } if (inputURL.indexOf('://') == -1 && inputURL.indexOf('mailto:') == -1) { inputURL = 'http://' + inputURL; } if (inputURL.indexOf('http://') != 0 && inputURL.indexOf('https://') != 0 && inputURL.indexOf('ftp://') != 0 && inputURL.indexOf('file://') != 0 && inputURL.indexOf('mailto:') != 0) { return; } if (inputURL == 'http://' || inputURL == 'https://' || inputURL == 'ftp://' || inputURL == 'file://' || inputURL == 'mailto:') { return; } if (isNewTab) { window.open(url); } else { var page = this.paramsParser.getURLPDFParams(url).page; if (page) this.viewport_.goToPage(page); else window.location.href = url; } } https://codereview.chromium.org/830433002/diff/260001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/830433002/diff/260001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:137: const char kJSSetNamedDestinations[] = "setNamedDestinations"; nit kJSSetNamedDestinationsType https://codereview.chromium.org/830433002/diff/260001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/830433002/diff/260001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:2345: unsigned long name_length = buffer_bytes / bytes_in_char16; nit: merge the above 2 lines https://codereview.chromium.org/830433002/diff/260001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:2346: if (name_length > 1) { this should just be name_length > 0 when switching to use the class mentioned below https://codereview.chromium.org/830433002/diff/260001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:2348: buffer_bytes); thestig just added a class which will make doing this right safer. PTAL at https://codereview.chromium.org/848073003/diff/60001/pdf/pdfium/pdfium_api_st...
Except thestig change usage, other changes done as suggested. I will do changes once thestig changes rolled in. PTAL https://codereview.chromium.org/830433002/diff/260001/chrome/browser/resource... File chrome/browser/resources/pdf/open_pdf_params_parser.js (right): https://codereview.chromium.org/830433002/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:58: getURLPDFParams: function(inputUrl) { On 2015/01/15 04:51:11, raymes wrote: > nit: let's change the name because I think it is doing something different. > getViewportFromUrlParams Done. https://codereview.chromium.org/830433002/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:68: } On 2015/01/15 04:51:11, raymes wrote: > I don't think we need this whole if-block Then how we will handle http://XXX.pdf#Chapter5 case. where we need to mode to 'Chapter5' directly as a part of load. and even in the current issue we require page number for #US, that is coming from this.namedDestinations[]. If we can achieve this without above code, please suggest. https://codereview.chromium.org/830433002/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:81: this.urlParams['nameddest'] = pageNumber; On 2015/01/15 04:51:11, raymes wrote: > Can we just store this in the 'page' param as well? Done. https://codereview.chromium.org/830433002/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:87: if (!isNaN(pageNumber) && pageNumber > 0) On 2015/01/15 04:51:11, raymes wrote: > good catch! thanks :) Acknowledged. https://codereview.chromium.org/830433002/diff/260001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/830433002/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:139: this.paramsParser = new OpenPDFParamsParser(); On 2015/01/15 04:51:11, raymes wrote: > Let's keep this named paramsParser_. An underscore at the end means it's private > to the class Done. https://codereview.chromium.org/830433002/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:291: this.viewport_.goToPage(urlParams.nameddest); On 2015/01/15 04:51:11, raymes wrote: > If we merge nameddest and page we can remove this if-statement Done. https://codereview.chromium.org/830433002/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:353: navigate_: function(urlFragment, isNewTab) { On 2015/01/15 04:51:11, raymes wrote: > I'd like to start reducing this size of this class so can we move this function > into a separate class/file? We can call it URLNavigator or something and it > would just have a single function which is navigate. I understand, I will do refactoring in a separate CL, Let this CL rolled in. https://codereview.chromium.org/830433002/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:393: chrome.tabs.create({ url: inputURL }); On 2015/01/15 04:51:11, raymes wrote: > Cool, thanks for looking into that! Let's leave it as you have it for now then > :) Acknowledged. https://codereview.chromium.org/830433002/diff/260001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:403: }, On 2015/01/15 04:51:11, raymes wrote: > Here is what I was thinking (there are a few changes from what you have). I've > stripped out all the comments but please add them back in! :) > > navigate_: function(url, isNewTab) { > if (url.length == 0) > return; > > var originalUrl = this.streamDetails_.originalUrl; > > if (url[0] == '#') { > var hashIndex = originalUrl.search('#') > if (hashIndex != -1) > url = originalUrl.substring(0, hashIndex) + url > else > url = originalUrl + url > } > > if (inputURL.indexOf('://') == -1 && inputURL.indexOf('mailto:') == -1) { > inputURL = 'http://' + inputURL; > } > > if (inputURL.indexOf('http://') != 0 && > inputURL.indexOf('https://') != 0 && > inputURL.indexOf('ftp://') != 0 && > inputURL.indexOf('file://') != 0 && > inputURL.indexOf('mailto:') != 0) { > return; > } > > if (inputURL == 'http://' || > inputURL == 'https://' || > inputURL == 'ftp://' || > inputURL == 'file://' || > inputURL == 'mailto:') { > return; > } > > if (isNewTab) { > window.open(url); > } else { > var page = this.paramsParser.getURLPDFParams(url).page; > if (page) > this.viewport_.goToPage(page); > else > window.location.href = url; > } > } Done. https://codereview.chromium.org/830433002/diff/260001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/830433002/diff/260001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:137: const char kJSSetNamedDestinations[] = "setNamedDestinations"; On 2015/01/15 04:51:11, raymes wrote: > nit kJSSetNamedDestinationsType Done. https://codereview.chromium.org/830433002/diff/260001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/830433002/diff/260001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:2345: unsigned long name_length = buffer_bytes / bytes_in_char16; On 2015/01/15 04:51:11, raymes wrote: > nit: merge the above 2 lines Done. https://codereview.chromium.org/830433002/diff/260001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:2346: if (name_length > 1) { On 2015/01/15 04:51:11, raymes wrote: > this should just be name_length > 0 when switching to use the class mentioned > below Done. https://codereview.chromium.org/830433002/diff/260001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:2348: buffer_bytes); On 2015/01/15 04:51:11, raymes wrote: > thestig just added a class which will make doing this right safer. PTAL at > https://codereview.chromium.org/848073003/diff/60001/pdf/pdfium/pdfium_api_st... These changes are not yet rolled in.Once These rolled in I will do required modifications.
@raymes, I have done changes and verified with thestig patch. It is working as expected. PTAL.
Mostly nits now. Thanks! https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... File chrome/browser/resources/pdf/open_pdf_params_parser.js (right): https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:13: this.urlParams = {}; let's remove this as an instance variable and make it a local variable https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:26: parseZoomParam_: function(paramValue) { This function can take the urlParams as an argument to make it a local variable https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:55: * @param {string} inputUrl that need to be parsed. nit: need->needs nit: inputUrl->url https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:56: * @return {Object} urlParams data that have the PDF url parameter info. nit: @return {Object} A dictionary containing the viewport which should be displayed based on the URL https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:58: getViewportFromUrlParams: function(inputUrl) { nit: inputUrl->url https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:60: this.urlParams = {}; var urlParams = {}; https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:61: var originalUrl = inputUrl; nit this isn't needed, just always use "url" https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:81: var pageNumber = parseInt(paramsDictionary['nameddest']); paramsDictionary['nameddest'] isn't expected to be a number, it's expected to be a named dest which we look up. e.g. var page = paramsDictionary['nameddest'] if (page) this.urlParams['page'] = page; https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:347: * within the PDF page or opening a new url (in the same tab or a new tab) nit: '.' at the end of hte sentence https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:348: * @param {string} message data that have the url for navigation. You need to include the name of the param here. e.g. @param {string} url The URL to navigate to. @param {boolean} newTab Whether to perform the navigation in a new tab or in the current tab. https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:352: navigate_: function(url, isNewTab) { nit: isNewTab->newTab https://codereview.chromium.org/830433002/diff/320001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/830433002/diff/320001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:2353: std::string name_dest = base::UTF16ToUTF8(name); nit: name_dest->named_dest
https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... File chrome/browser/resources/pdf/open_pdf_params_parser.js (right): https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:67: if ((paramTokens.length == 1) && (paramTokens[0].search('=') == -1)) { nit: Let's add the same comment here as in instance.cc (you might have had this before but I got confused, sorry!!) // Handle the case of http://foo.com/bar#NAMEDDEST. This is not explicitly // mentioned except by example in the Adobe "PDF Open Parameters" document.
Changes done as suggested. PTAL. Thanks https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... File chrome/browser/resources/pdf/open_pdf_params_parser.js (right): https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:13: this.urlParams = {}; On 2015/01/15 23:07:42, raymes wrote: Done. https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:26: parseZoomParam_: function(paramValue) { On 2015/01/15 23:07:42, raymes wrote: Done. https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:55: * @param {string} inputUrl that need to be parsed. On 2015/01/15 23:07:43, raymes wrote: Done. https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:56: * @return {Object} urlParams data that have the PDF url parameter info. On 2015/01/15 23:07:42, raymes wrote: Done. https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:58: getViewportFromUrlParams: function(inputUrl) { On 2015/01/15 23:07:43, raymes wrote: Done. https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:60: this.urlParams = {}; On 2015/01/15 23:07:43, raymes wrote: Done. https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:61: var originalUrl = inputUrl; On 2015/01/15 23:07:42, raymes wrote: Done. https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:67: if ((paramTokens.length == 1) && (paramTokens[0].search('=') == -1)) { On 2015/01/15 23:08:37, raymes wrote: Done. https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:81: var pageNumber = parseInt(paramsDictionary['nameddest']); On 2015/01/15 23:07:42, raymes wrote: Done. https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:347: * within the PDF page or opening a new url (in the same tab or a new tab) On 2015/01/15 23:07:43, raymes wrote: Done. https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:348: * @param {string} message data that have the url for navigation. On 2015/01/15 23:07:43, raymes wrote: Done. https://codereview.chromium.org/830433002/diff/320001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:352: navigate_: function(url, isNewTab) { On 2015/01/15 23:07:43, raymes wrote: Done. https://codereview.chromium.org/830433002/diff/320001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/830433002/diff/320001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:2353: std::string name_dest = base::UTF16ToUTF8(name); On 2015/01/15 23:07:43, raymes wrote: Done.
lgtm with the following comments https://codereview.chromium.org/830433002/diff/360001/chrome/browser/resource... File chrome/browser/resources/pdf/open_pdf_params_parser.js (right): https://codereview.chromium.org/830433002/diff/360001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:60: var urlParams = {}; nit: let's rename this viewportPosition https://codereview.chromium.org/830433002/diff/360001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:83: var page = paramsDictionary['nameddest']; Sorry I got this wrong last time. What I meant to say is: var page = this.namedDestinations[paramsDictionary['nameddest']]; Could you test to make sure it works? Thanks! https://codereview.chromium.org/830433002/diff/360001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/830433002/diff/360001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:286: var urlParams = nit: let's rename this viewportPosition
Please add the following in followup CLs: 1) Refactoring which pulls navigate_ into a different file/class 2) A test for checking that various named parameters trigger navigation to the correct place in the document. Have a look at https://codereview.chromium.org/840493002/ to get a rough idea of how to add a test.
And thanks for working on this!
Thanks for review. I have done changes as suggested, and tested, It is working as expected. https://codereview.chromium.org/830433002/diff/360001/chrome/browser/resource... File chrome/browser/resources/pdf/open_pdf_params_parser.js (right): https://codereview.chromium.org/830433002/diff/360001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:60: var urlParams = {}; On 2015/01/18 22:01:39, raymes wrote: > nit: let's rename this viewportPosition Done. https://codereview.chromium.org/830433002/diff/360001/chrome/browser/resource... chrome/browser/resources/pdf/open_pdf_params_parser.js:83: var page = paramsDictionary['nameddest']; On 2015/01/18 22:01:39, raymes wrote: > Sorry I got this wrong last time. What I meant to say is: > var page = this.namedDestinations[paramsDictionary['nameddest']]; > > Could you test to make sure it works? Thanks! I have checked with http://examples.itextpdf.com/results/part1/chapter02/movie_links_1.pdf#namedd... Here it will go to page 23, as we are getting UY value as 22 and page starts from index 0. So it is working as expected. Thanks
On 2015/01/18 22:05:09, raymes wrote: > Please add the following in followup CLs: > 1) Refactoring which pulls navigate_ into a different file/class > 2) A test for checking that various named parameters trigger navigation to the > correct place in the document. Have a look at > https://codereview.chromium.org/840493002/ to get a rough idea of how to add a > test. yes, sure, I will raises a followup issue for these to start work on these items. Thanks for review.
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/830433002/380001
The CQ bit was unchecked by deepak.m1@samsung.com
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/830433002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) ios_rel_device_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...) ios_rel_device_ninja_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/830433002/450001
Message was sent while issue was closed.
Committed patchset #24 (id:450001)
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/6313ce26ea190d5da88e4fe6473b693de37305e2 Cr-Commit-Position: refs/heads/master@{#312075} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
