New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for pdf in sub frames (https://github.com/electron/electr… #10793

Merged
merged 16 commits into from Nov 20, 2017

Conversation

Projects
None yet
6 participants
@ahmedmohamedali
Copy link
Contributor

ahmedmohamedali commented Oct 13, 2017

This PR enable the PDF rendering in sub frame (Issue #9192).
It work with the chromium changes described in the PR electron/libchromiumcontent#374.

@ahmedmohamedali ahmedmohamedali requested a review from electron/reviewers as a code owner Oct 13, 2017

@welcome

This comment has been minimized.

Copy link

welcome bot commented Oct 13, 2017

💖 Thanks for opening this pull request! 💖

Here is a list of things that will help get it across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.
    We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.
@deepak1556
Copy link
Member

deepak1556 left a comment

Can you run clang-format to fix the coding style. Looks good to me, otherwise. Thanks!

int render_process_host_id;
int render_frame_id;
info->GetAssociatedRenderFrame(&render_process_host_id, &render_frame_id);
content::RenderFrameHost* rfh =

This comment has been minimized.

@deepak1556

deepak1556 Oct 14, 2017

Member

RenderFrameHost methods should only be accessed on the UI thread.


int render_process_host_id;
int render_frame_id;
info->GetAssociatedRenderFrame(&render_process_host_id, &render_frame_id);

This comment has been minimized.

@deepak1556

deepak1556 Oct 14, 2017

Member

Better to verify the result of GetAssociatedRenderFrame method before proceeding with retrieving the frame_tree_node_id.

ahmedmohamedali added some commits Oct 14, 2017

Applying changes requested by @deepak1556 after the review:
- Move RenderFrameHost methods in the UI thread
- Check GetAssociatedRenderFrame return value
@ahmedmohamedali

This comment has been minimized.

Copy link
Contributor

ahmedmohamedali commented Oct 16, 2017

Hi @deepak1556,
Can you please review the changes I made after your first review ?
Thanks.

@deepak1556

This comment has been minimized.

Copy link
Member

deepak1556 commented Oct 17, 2017

@MarshallOfSound

This comment has been minimized.

Copy link
Member

MarshallOfSound commented Oct 17, 2017

Looks like line endings were brutalized in this file 🤔

@ahmedmohamedali

This comment has been minimized.

Copy link
Contributor

ahmedmohamedali commented Oct 17, 2017

@deepak1556 ,

I'm not familiar with electron test environment. Can you tell me how to run the tests or point me to a document explaining how to run them ? Thanks

@MarshallOfSound ,

I have formatted the file with clang-format as suggested by @deepak1556.
It isn't enough ? Thanks.

@deepak1556

This comment has been minimized.

@ckerr

This comment has been minimized.

Copy link
Member

ckerr commented Oct 17, 2017

Changing the linefeeds makes the entire file show up in the diff.

It's possible this is a side-effect of running clang-format on Windows, that it's rewriting the file with Windows linefeeds instead of what was there before?

There are a couple of ways to change it back, e.g.

  • With perl, you can change them back with perl -i -p -e "s/\r//" <filename> [<filename2> ...]

  • In notepad++, you can run Edit > EOL Conversion > UNIX/OSX Format

This would be after running clang-format, of course 😄

After doing this, diffing your changes to master should just show the lines changed by adding pdf support in subframes

@ahmedmohamedali

This comment has been minimized.

Copy link
Contributor

ahmedmohamedali commented Oct 18, 2017

thanks @ckerr,

I tried to fix the linefeeds with notepad++ but I found it is the git commit which is converting LF back to CRLF in windows.

@ahmedmohamedali ahmedmohamedali requested a review from electron/printing as a code owner Oct 19, 2017

Fix crash that happens when the PDF viewer is refreshed.
The root cause is the PdfViewerHandler instanceis destroyed but not removed from the list of observer in WebContentsZoomController
@ahmedmohamedali

This comment has been minimized.

Copy link
Contributor

ahmedmohamedali commented Oct 19, 2017

Thanks @deepak1556. I have added the following two test cases

  • PDF in a sub frame
  • PDF in a nested sub frame

It is expected those two test to fail until the PR electron/libchromiumcontent#374 is integrated in electron.

@ahmedmohamedali

This comment has been minimized.

Copy link
Contributor

ahmedmohamedali commented Oct 24, 2017

Hi @deepak1556,
Is there anything missing for going forward with this PR ? Thanks.

ahmedmohamedali added some commits Nov 8, 2017

@ahmedmohamedali

This comment has been minimized.

Copy link
Contributor

ahmedmohamedali commented Nov 11, 2017

Now that electron/libchromiumcontent#374 is merged, the tests should pass.
Closing and re-opening this PR to trigger a CI build.

</body>
</html>


This comment has been minimized.

@alexeykuzmin

alexeykuzmin Nov 12, 2017

Contributor

Empty lines are not needed here. In this file and in the html file above.
We should probably have a linter doing this kind of checks =/

@@ -0,0 +1,7 @@
<html>
<body>
<iframe id='outer-frame' src="./pdf-in-iframe.html"/>

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Nov 12, 2017

Contributor

Please use double quotes for attributes.

@@ -1012,8 +1034,8 @@ describe('chromium feature', () => {
w.webContents.loadURL(pdfSource)
})

it('should not open when pdf is requested as sub resource', (done) => {
createBrowserWindow({plugins: true})
it('should not open when pdf is requested as sub resource', function (done) {

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Nov 12, 2017

Contributor

Why (done) => was changed to function (done)?

This comment has been minimized.

@ahmedmohamedali

ahmedmohamedali Nov 13, 2017

Contributor

@alexeykuzmin,
function (done) was removed after my fork with the following change
[WIP] Upgrade more specs (#10945)
e4214a6

I think I have also missed it during the conflict resolution. Will fix it.

})

createBrowserWindow({plugins: true, preload: preloadFile})
ipcMain.once('pdf-loaded', function (event, state) {

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Nov 12, 2017

Contributor

Is it possible that 'pdf-loaded' listener will be called before the one of 'page-title-updated'?

This comment has been minimized.

@deepak1556

deepak1556 Nov 13, 2017

Member

Its possible if the title is updated in javascript. But it shouldn't happen for this spec.

@alexeykuzmin

This comment has been minimized.

Copy link
Contributor

alexeykuzmin commented Nov 12, 2017

Now that electron/libchromiumcontent#374 is merged, the tests should pass.

@ahmedmohamedali, you need to update vendor/libchromiumcontent submodule reference to use a new libchromiumcontent revision.

@zcbenz

This comment has been minimized.

Copy link
Contributor

zcbenz commented Nov 14, 2017

We are having some problems with libchromiumcontent binaries not being uploaded, so the tests are currently failing because of that.

@zcbenz

zcbenz approved these changes Nov 20, 2017

Copy link
Contributor

zcbenz left a comment

👍

looks like the op has met all requested changes

@zcbenz zcbenz merged commit cd5785c into electron:master Nov 20, 2017

6 checks passed

ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

thomasLechaptois added a commit to thomsonreuters/electron that referenced this pull request Dec 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment