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

frame navigation API #11607

Merged
merged 3 commits into from Feb 26, 2018

Conversation

Projects
None yet
5 participants
@bughit
Contributor

bughit commented Jan 9, 2018

a couple of things I'm uncertain about

  1. can WebFrame* be cast to WebLocalFrame* (it's working empirically)
  2. if there is a better way to do string conversions (v8:String => blink::WebString)

@bughit bughit requested a review from electron/reviewers as a code owner Jan 9, 2018

@welcome

This comment has been minimized.

welcome bot commented Jan 9, 2018

💖 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.
@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Jan 9, 2018

Hey @bughit, struggling to review this because all the line-endings changed. Can you fix those line-endings please 👍

@bughit

This comment has been minimized.

Contributor

bughit commented Jan 10, 2018

@MarshallOfSound fixed

I initally create a .git\info\attributes with * text=auto which I thought would convert to lf on commit, but it didn't somehow

@bughit

This comment has been minimized.

Contributor

bughit commented Jan 10, 2018

@MarshallOfSound could you please comment on the following:

  1. can WebFrame* be cast to WebLocalFrame* (i.e. they are always WelLocalFrames) as I am doing in all methods but GetFrameForSelector.
    1. GetFrameForSelector is able to obtain a WebLocalFrame* via blink::WebLocalFrame::FromFrameOwnerElement which kind of implies that they are indeed always WebLocal
    2. What are the WebRemoteFrames for?
  2. if there is a better way to do string conversions (v8:String => blink::WebString) than what I'm doing in GetFrameForSelector and FindFrameByName
@@ -37,7 +34,9 @@ class WebFrame : public mate::Wrappable<WebFrame> {
v8::Local<v8::FunctionTemplate> prototype);
private:
explicit WebFrame(v8::Isolate* isolate);
explicit WebFrame(v8::Isolate* isolate,

This comment has been minimized.

@deepak1556

deepak1556 Jan 11, 2018

Member

Its better to have two versions of WebFrame constructors rather than having an implicit definition for clarity.

WebFrame(v8::Isolate* isolate);
WebFrame(v8::Isolate* isolate, blink::WebFrame* web_frame);
return v8::Null(isolate());
}
v8::Local<v8::Value> WebFrame::FindFrameByName(v8::Local<v8::Value> name_value,

This comment has been minimized.

@deepak1556

deepak1556 Jan 11, 2018

Member

You can extract the name or selector values directly from mate::Arguments which performs the right string conversion.

v8::Local<v8::Value> WebFrame::FindFrameByName(mate::Arguments* args) {
  std::string name;
  if (args && args->Length() == 1 && args->GetNext(&name) {}
}
@@ -368,6 +368,116 @@ void WebFrame::ClearCache(v8::Isolate* isolate) {
base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL);
}
v8::Local<v8::Value> WebFrame::Opener() const {
blink::WebLocalFrame* local_frame =
(blink::WebLocalFrame*) WebFrame::web_frame_->Opener();

This comment has been minimized.

@deepak1556

deepak1556 Jan 11, 2018

Member

Its possible for a blink::WebFrame to be either blink::WebLocalFrame or blink::WebRemoteFrame when traversing frames across process, this model isn't currently enabled in Electron but will soon be. So its better to prepare for it by using the helpers blink::WebFrame::IsWebLocalFrame and blink::WebFrame::ToWebLocalFrame

@bughit

This comment has been minimized.

Contributor

bughit commented Jan 11, 2018

@deepak1556 updated

@bughit

This comment has been minimized.

Contributor

bughit commented Jan 12, 2018

Is there anything else I need to do here?

@bughit

This comment has been minimized.

Contributor

bughit commented Jan 17, 2018

@deepak1556, I made the changes you requested almost a week ago, could you please review them.

@bughit

This comment has been minimized.

Contributor

bughit commented Jan 29, 2018

@MarshallOfSound @deepak1556

Could one of you please explain what I can expect to happen with this pull request? It's been sitting in the "Changes requested" state for 18 days after requested changes were made. Is that not enough time to let me know if the changes are acceptable or if you need anything else?

@deepak1556

This comment has been minimized.

Member

deepak1556 commented Feb 1, 2018

@bughit sorry for the delay on this. I am not convinced on exposing the traversal methods on WebFrame which creates unnecessary instances. We can just add one method to retrieve all WebElement of a frame and another method to return a WebFrame instance for given WebElement.

@bughit

This comment has been minimized.

Contributor

bughit commented Feb 1, 2018

@deepak1556

Please look at #5115 (comment) and following. I initially tried that approach and could not figure out how to get a blink::WebElement from a v8::Value (#5115 (comment)), got as far as blink::Element.

The current approach resulted as workaround. I am not opposed to the original way (webelement -> webframe), but I would need to know how.

@bughit

This comment has been minimized.

Contributor

bughit commented Feb 5, 2018

@deepak1556

If we are going to communicate with a periodicity of 3 weeks, this will never get done.

Besides not being clear on what exactly you want, I don't understand your objection. The api I exposed is:

  • v8::Localv8::Value Opener() const;
  • v8::Localv8::Value Parent() const;
  • v8::Localv8::Value Top() const;
  • v8::Localv8::Value FirstChild() const;
  • v8::Localv8::Value NextSibling() const;
  • v8::Localv8::Value TraverseNext() const;
  • v8::Localv8::Value GetFrameForSelector(mate::Arguments* args) const;
  • v8::Localv8::Value FindFrameByName(mate::Arguments* args) const;

Opener, Parent, Top, GetFrameForSelector, FindFrameByName are definitely useful and don't create any unnecessary instances. Do you have anything against these?

FirstChild, NextSibling, TraverseNext only create unnecessary instances if you are using them to achieve array like indexing (e.g. 5th child). In other cases (collecting the frames, or searching through them) they are not unnecessary. Even in the indexing scenario the the extra instances are hardly significant (pages which have so many frames where this might become an issue are theoretical)

@zcbenz

And can you add some documents for the new APIs in docs/api/web-frame.md?

@@ -368,6 +374,102 @@ void WebFrame::ClearCache(v8::Isolate* isolate) {
base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL);
}
v8::Local<v8::Value> WebFrame::Opener() const {
blink::WebLocalFrame* local_frame =
WebFrame::web_frame_->Opener()->ToWebLocalFrame();

This comment has been minimized.

@zcbenz

zcbenz Feb 7, 2018

Contributor

ToWebLocalFrame would cause assertion under Debug mode when called for remote web frame, we should call IsWebLocalFrame() before doing conversion.

All the ToWebLocalFrame calls bellow should also be checked.

This comment has been minimized.

@bughit

bughit Feb 7, 2018

Contributor

I would like to understand when a WebFrame is local vs remote.
Does it depend on the chosen process model?
What's the current model? Process per window (which would mean all frames are local)?
Is the process model going to change (so that not all frames are local)?
Will it be possible to configure the process model?

And now to the details, what should these methods return if the frame is not local?

This comment has been minimized.

@zcbenz

zcbenz Feb 12, 2018

Contributor

Regardless of process model, it is always possible to have a remote WebFrame inside any page , especially after Chromium started to enable out-of-process iframes.

I think for now it is fine to return null for remote WebFrame.

v8::Local<v8::Value> WebFrame::TraverseNext() const {
blink::WebLocalFrame* local_frame =
WebFrame::web_frame_->TraverseNext()->ToWebLocalFrame();

This comment has been minimized.

@zcbenz

zcbenz Feb 7, 2018

Contributor

The travel would stop when encountered a remote frame. Also TraverseNext returns nullptr at the end so it would then crash here.

If we do not really need this API, I prefer to simply remove this.

This comment has been minimized.

@bughit

bughit Feb 12, 2018

Contributor

I included it just because it was there, so I am ok with removing it, firstChild and nextSibling should enough to walk the whole tree.

return v8::Null(isolate());
}
v8::Local<v8::Value> WebFrame::GetFrameForSelector(mate::Arguments* args)

This comment has been minimized.

@zcbenz

zcbenz Feb 7, 2018

Contributor

You can define the function as GetFrameForSelector(const std::string& selector) and the argument would be converted automatically.

This also applies to FindFrameByName.

@bughit bughit requested a review from electron/docs as a code owner Feb 21, 2018

@bughit

This comment has been minimized.

Contributor

bughit commented Feb 21, 2018

@zcbenz I made the requested changes. The docs linting is failing, I am not clear on how to fix it, please advise.

@@ -12,9 +16,9 @@ const {webFrame} = require('electron')
webFrame.setZoomFactor(2)
```
## Methods

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Feb 21, 2018

Member

This is why linting has failing, if you revert this back to Methods everything should work. webFrame is not a class, it's a module (that happens to be an the instance of a native WebFrame class) but it's a module 😄

This comment has been minimized.

@bughit

bughit Feb 21, 2018

Contributor

I'll revert it to fix the problem, but webFrame is not a module, unless Electron has its own concept of modules.

  • it is an exported object of the electron module require('electron').webFrame
  • it is an instance of the WebFrame class (which is not exported because you can't create new frames)
  • it represents the top frame of the current window
  • its methods are instance methods of the WebFrame class
  • till now it was a singleton (the only WebFrame instance you could obtain) but with this pull request you can get others

This comment has been minimized.

@bughit

bughit Feb 22, 2018

Contributor

Went back to Methods, still failing: Failed to lint electron.d.ts

@bughit

This comment has been minimized.

Contributor

bughit commented Feb 22, 2018

@zcbenz @deepak1556

I added the documentation per your request, but I don't know what it takes to satisfy your linter (Failed to lint electron.d.ts). Please advise.

@bughit

This comment has been minimized.

Contributor

bughit commented Feb 23, 2018

@MarshallOfSound @deepak1556 @zcbenz

Do none of you know what's wrong with the documentation I added? I can take it out, but you asked for it, so why not help me get it right?

@qazbnm456

This comment has been minimized.

Contributor

qazbnm456 commented Feb 26, 2018

@bughit The linting has failed because you don't follow the style guide mentioned here I think. 🤔

Hopefully, amending the docs of properties of the webFrame may make the linting happy.

@bughit

This comment has been minimized.

Contributor

bughit commented Feb 26, 2018

@qazbnm456 Is it just the backquotes? Or are properties not allowed at all for "modules"?

Speaking of which, where does the electron concept of a module come from? From the node perspective webFrame is not a module.

  • it is an exported object of the electron module require('electron').webFrame
  • it is an instance of the WebFrame class require('electron').webFrame.constructor.name, which is not exported directly because you can't create new frames
  • it represents the top frame of the current window
  • its methods are instance methods of the WebFrame class
@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Feb 26, 2018

@bughit The docs parser is treating Returns as a keyword in method description to get the return type, so it is using its own conceptions when parsing the docs, instead of following common conceptions which only humans can do. I'll fix the docs parser.

zcbenz added some commits Feb 26, 2018

@zcbenz

zcbenz approved these changes Feb 26, 2018

fixed now

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Feb 26, 2018

The CI is a bit flaky and the failing tests are not related to this PR, I'm merging with the failing tests ignored.

@zcbenz zcbenz merged commit fdd66bd into electron:master Feb 26, 2018

7 of 9 checks passed

continuous-integration/jenkins/pr-head This commit cannot be built
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test 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
@welcome

This comment has been minimized.

welcome bot commented Feb 26, 2018

Congrats on merging your first pull request! 🎉🎉🎉

@bughit

This comment has been minimized.

Contributor

bughit commented Feb 28, 2018

@zcbenz Can this be added to 2-0-x?

Would like to be able to start using this sooner. Also master can't even be built on windows currently (#12061)

@bughit

This comment has been minimized.

Contributor

bughit commented Mar 1, 2018

@zcbenz Is backporting to a beta branch a possibility?

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Mar 1, 2018

According our versioning policy, you have to wait for major/minor release for a new feature.

@bughit bughit deleted the bughit:frame_navigation_api branch May 1, 2018

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