Skip to content
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.

Sign up for GitHub

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

Jump to bottom

webview: page-favicon-updated navigation event and getFavicon api #1438

Merged
merged 5 commits into from
Apr 25, 2015
Merged

webview: page-favicon-updated navigation event and getFavicon api #1438

merged 5 commits into from
Apr 25, 2015

Conversation

deepak1556
Copy link
Member

Related #1378 and #1079 , not quite sure on how to achieve the various mutations for page-url-set. @zcbenz any clue on why the test for will-navigate gets called for spec/index.html too ?

Depends on electron/native-mate#1

steelbrain
steelbrain reviewed Apr 17, 2015
View reviewed changes
docs/api/browser-window.md
* `event` Event
* `favicons` [String]

Emitted when page receives favicon urls.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"urls" as far as I know, a page only has one favicon, which means only one wrong.
Was it introduced recently?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can specify icons with various sizes, this method will fetch all favicon urls present for a site. Maybe i should provide [size] also as payload. I made the event name a bit confusing, will fix it and add api to get the current active favicon

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that I had a very confusing typo in there 😆

@deepak1556 deepak1556 changed the title webview: will-navigate and page-favicon-set navigation events webcontents: will-navigate, did-navigate and page-favicon-updated navigation events Apr 20, 2015
@deepak1556 deepak1556 changed the title webcontents: will-navigate, did-navigate and page-favicon-updated navigation events webview: will-navigate, did-navigate and page-favicon-updated navigation events Apr 20, 2015
@deepak1556
Copy link
Member Author

Added getFavicon api to retrieve current favicon as nativeimage. Also have worked around the webview spec issue.

@deepak1556 deepak1556 force-pushed the api_web_view_patch branch 2 times, most recently from b342048 to d8dbb7b Compare April 23, 2015 11:34
zcbenz
zcbenz reviewed Apr 25, 2015
View reviewed changes
atom/browser/api/atom_api_web_contents.h
@@ -6,13 +6,17 @@
#define ATOM_BROWSER_API_ATOM_API_WEB_CONTENTS_H_

#include <string>
#include <vector>
#include <set>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need to include set in header.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cpplint shouts otherwise :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm this is strange, which line was it complaining? The atom_api_web_contents.cc should still need to include set though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah its because of that, but i decided to have the standard headers in one place in api_web_contents.h , should i move it ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it should be moved, api_web_contents.h is also included by other files, adding a new include in api_web_contents.h would slow down compilation speed for all those files. Even though it only slows down a little, it is still a good habit to keep minimal includes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah cool, thanks!

@zcbenz
Copy link
Member

zcbenz commented Apr 25, 2015

@zcbenz any clue on why the test for will-navigate gets called for spec/index.html too ?

I have no idea, I'm surprised that it happened.

@zcbenz
Copy link
Member

zcbenz commented Apr 25, 2015

I don't think we should add did-navigate event for now, since the history control (#1249) is still a mess.

And for the will-navigate event, it seems that a navigation happened in the webview page won't trigger it?

@deepak1556
Copy link
Member Author

About did-navigate , right now i m triggering it only if its a navigation to a different page than the previous , so the reload entry behaviour shouldnt affect it load_details.is_navigation_to_different_page() right ?

You need will-navigate to trigger after a navigation ?

@deepak1556
Copy link
Member Author

One caveat with current did-navigate is , it wont trigger for in page navigation (eg: link based). Is that fine ?

@deepak1556
Copy link
Member Author

Made the changes except for did-navigate removal and pending doubt on will-navigate event.

zcbenz
zcbenz reviewed Apr 25, 2015
View reviewed changes
atom/browser/api/atom_api_web_contents.cc
if (!entry)
return gfx::Image();
auto favicon_status = entry->GetFavicon();
return favicon_status.image;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can simply do return entry->GetFavicon().image.

@zcbenz
Copy link
Member

zcbenz commented Apr 25, 2015

One caveat with current did-navigate is , it wont trigger for in page navigation (eg: link based). Is that fine ?

The purpose of did-navigate event is to write browsers, so triggering for page navigations is required.

You need will-navigate to trigger after a navigation ?

The will-naviage event is used for preventing users to visit certain links, it should be triggered when users click a link in the webview.

@zcbenz
Copy link
Member

zcbenz commented Apr 25, 2015

We should probably move did-navigate and will-navigate to separate PRs since it seems to take much more work to make them full functional.

@deepak1556
Copy link
Member Author

Ah now its clear , thanks! have removed them, will try a new PR later.

@deepak1556 deepak1556 changed the title webview: will-navigate, did-navigate and page-favicon-updated navigation events webview: page-favicon-updated navigation event and getFavicon api Apr 25, 2015
zcbenz added a commit that referenced this pull request Apr 25, 2015
@zcbenz
Merge pull request #1438 from deepak1556/api_web_view_patch
1649d8f 
webview: page-favicon-updated navigation event and getFavicon api
@zcbenz zcbenz merged commit 1649d8f into electron:master Apr 25, 2015
@zcbenz zcbenz mentioned this pull request Apr 26, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@deepak1556 @zcbenz @steelbrain