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 showDefinitionForSelection to webContents/webview #5921

Merged
merged 8 commits into from Jun 8, 2016

Conversation

Projects
None yet
2 participants
@kevinsawicki
Contributor

kevinsawicki commented Jun 7, 2016

Adds the Mac OS X only showDefinitionForSelection API to webContents and <webview> that is currently only on the BrowserWindow API.

Also adds a TODO to deprecate BrowserWindow.showDefinitionForSelection in 2.0 since it seems consistent with the other webContents APIs like findInPage, replaceMisspelling, etc.

Closes #5275

@@ -379,7 +379,8 @@ var registerWebViewElement = function () {
'downloadURL',
'inspectServiceWorker',
'print',
'printToPDF'
'printToPDF',
'showDefinitionForSelection'

This comment has been minimized.

@kevinsawicki

kevinsawicki Jun 7, 2016

Contributor

@zcbenz do you think we should still be adding new APIs to <webview> that just pass through to webContents APIs or should this be accessible only via <webview>.getWebContents().showDefinitionForSelection()?

This comment has been minimized.

@zcbenz

zcbenz Jun 8, 2016

Contributor

Adding new APIs to <webview> allows us to do some optimizations, <webview>.getWebContents() is too heavy.

if (view)
view->ShowDefinitionForSelection();
#else
NOTIMPLEMENTED();

This comment has been minimized.

@zcbenz

zcbenz Jun 8, 2016

Contributor

NOTIMPLEMENTED() means this code path should never be executed, it is a runtime check for programming errors, when linked with Debug build of libchromiumcontent it will throw assert error.

Leaving the implementation empty would be fine here.

This comment has been minimized.

@kevinsawicki

kevinsawicki Jun 8, 2016

Contributor

Oh awesome, good to know, thanks 👍

@@ -934,6 +934,8 @@ - (void)drawRect:(NSRect)dirtyRect {
}
void NativeWindowMac::ShowDefinitionForSelection() {

This comment has been minimized.

@zcbenz

zcbenz Jun 8, 2016

Contributor

We can remove this implementation and add a helper function in browser-window.js.

@kevinsawicki kevinsawicki merged commit f4c1cd1 into master Jun 8, 2016

7 of 9 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
electron-linux-arm Build #3397209 succeeded in 43s
Details
electron-linux-ia32 Build #3397210 succeeded in 38s
Details
electron-linux-x64 Build #3397211 succeeded in 118s
Details
electron-mas-x64 Build #1446 succeeded in 5 min 53 sec
Details
electron-osx-x64 Build #1451 succeeded in 6 min 17 sec
Details
electron-win-ia32 Build #447 succeeded in 6 min 14 sec
Details
electron-win-x64 Build #444 succeeded in 6 min 8 sec
Details

@kevinsawicki kevinsawicki deleted the webview-show-definition-for-selection branch Jun 8, 2016

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Jun 8, 2016

Thanks for the help on this one @zcbenz 👍 🚢

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