UrlBarIcon uses now only primitives #9754

NejcZdovc · 2017-06-28T12:40:13Z

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.

Resolves #9753

Auditors: @bsclifton @bridiver

Test Plan:

try to dnd lock image from url bar to the bookmark toolbar
new bookmark should be added

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

bridiver · 2017-06-29T19:57:39Z

app/renderer/components/navigation/urlBarIcon.js

@@ -111,7 +111,9 @@ class UrlBarIcon extends React.Component {

  onDragStart (e) {
    dndData.setupDataTransferURL(e.dataTransfer, this.props.location, this.props.title)
-    dndData.setupDataTransferBraveData(e.dataTransfer, dragTypes.TAB, this.props.activeFrame)
+    dndData.setupDataTransferBraveData(e.dataTransfer, dragTypes.TAB, Immutable.fromJS({


conversion to/from JS is a relatively expensive operation, why do we need to send an object instead of just the activeFrameKey value

Because this function expect data to Immutable https://github.com/brave/browser-laptop/blob/master/js/dndData.js#L26. What I can do is add a check in this function and if data is Immutable convert it, otherwise leave it as it is.

that function turns it right back into js again. I think we should stick with regular js

bridiver · 2017-06-29T21:15:35Z

js/dnd.js

-      appActions.addSite(bookmark, siteTags.BOOKMARK)
-    }
+    const frame = dndData.getDragData(dataTransfer, dragTypes.TAB)
+    windowActions.onFrameBookmark(frame.get('activeFrameKey'))


this change actually doesn't make sense to me. There's no state that is needed here so I don't see any reason to create a window action that forwards to an app action. We are using that anti-pattern in select places where there isn't a good workaround right now, but this doesn't seem necessary. What I think we should do (in another PR) is convert the drop methods to actions and move this code inside a (preferably browser process) reducer. It also seems like a bad idea to hide an action call inside this method and it should probably just return the bookmark from dndData and do nothing else. I think we're adding a layer of indirection here that is just going to be removed later anyway

Why I moved this into separate action is because we need to get detail of the frame so we can add it into sites.

Here we need to have access to the window state: https://github.com/brave/browser-laptop/pull/9754/files/e0827f5b092eecfbe915b23838fb9dccc8fdcc5b#diff-be01d0957f991e2400072d82d108450fL142

I think the confusing part here is that you're getting frame from dndData.getDragData, but apparently that isn't a frame so I think like the other one we should just pass frameKey. I just noticed this calls frame.get('activeFrameKey') which makes it even more confusing

Probably the naming is wrong here, so I fixed it. We send activeFrameKey from this line https://github.com/brave/browser-laptop/pull/9754/files/e0827f5b092eecfbe915b23838fb9dccc8fdcc5b#diff-51a114fecaac13cd41162f4c956e7508R114 and then get it back here as Immutable Map with this function

browser-laptop/js/dndData.js

Line 11 in 26a580b

module.exports.getDragData = (dataTransfer, dragType) => {

So we don't receive the whole frame, but we send end receive only frameKey

bridiver · 2017-07-05T15:49:36Z

app/renderer/components/navigation/urlBarIcon.js

@@ -111,7 +111,9 @@ class UrlBarIcon extends React.Component {

  onDragStart (e) {
    dndData.setupDataTransferURL(e.dataTransfer, this.props.location, this.props.title)
-    dndData.setupDataTransferBraveData(e.dataTransfer, dragTypes.TAB, this.props.activeFrame)
+    dndData.setupDataTransferBraveData(e.dataTransfer, dragTypes.TAB, {
+      activeFrameKey: this.props.activeFrameKey


sorry to do this to you, but can we use tabId instead of frameKey? If we had the tabId it would be easy to convert this to a browser process action (please add TODO for that) with a siteUtil.getDetailFromTabId method. I think we should drop active as well once it gets to this point

I totally agree and I forget about this one too frequently. Fixed

@bsclifton

Resolves brave#9753 Auditors: @bsclifton @bridiver Test Plan:

NejcZdovc · 2017-07-05T17:32:44Z

comment added

bsclifton · 2017-07-06T04:55:53Z

++

NejcZdovc requested review from bridiver and bbondy June 28, 2017 12:40

NejcZdovc self-assigned this Jun 28, 2017

NejcZdovc added the refactoring/redux-component label Jun 28, 2017

NejcZdovc added this to the 0.19.x (Nightly Channel) milestone Jun 28, 2017

NejcZdovc added the perf label Jun 28, 2017

NejcZdovc mentioned this pull request Jun 28, 2017

Only primitives in UrlBarIcon #9753

Closed

bridiver reviewed Jun 29, 2017

View reviewed changes

NejcZdovc force-pushed the refactor/#9753-urlbaricon branch 2 times, most recently from 48e1584 to e0827f5 Compare June 29, 2017 21:00

NejcZdovc requested a review from bridiver June 29, 2017 21:00

bridiver reviewed Jun 29, 2017

View reviewed changes

NejcZdovc force-pushed the refactor/#9753-urlbaricon branch from e0827f5 to 5bec429 Compare June 30, 2017 07:42

NejcZdovc requested a review from bridiver June 30, 2017 07:43

NejcZdovc force-pushed the refactor/#9753-urlbaricon branch from 5bec429 to a9f9b0b Compare June 30, 2017 11:28

NejcZdovc requested a review from bsclifton July 5, 2017 08:32

bridiver reviewed Jul 5, 2017

View reviewed changes

NejcZdovc force-pushed the refactor/#9753-urlbaricon branch from a9f9b0b to 1984a85 Compare July 5, 2017 16:03

NejcZdovc requested a review from bridiver July 5, 2017 16:03

UrlBarIcon uses now only primitives

1dec383

Resolves brave#9753 Auditors: @bsclifton @bridiver Test Plan:

NejcZdovc force-pushed the refactor/#9753-urlbaricon branch from 1984a85 to 1dec383 Compare July 5, 2017 17:32

NejcZdovc merged commit 4cc7d68 into brave:master Jul 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UrlBarIcon uses now only primitives #9754

UrlBarIcon uses now only primitives #9754

NejcZdovc commented Jun 28, 2017 •

edited

Loading

bridiver Jun 29, 2017

NejcZdovc Jun 29, 2017

bridiver Jun 29, 2017

NejcZdovc Jun 29, 2017

bridiver Jun 29, 2017 •

edited

Loading

NejcZdovc Jun 29, 2017

bridiver Jun 29, 2017

NejcZdovc Jun 30, 2017 •

edited

Loading

bridiver Jul 5, 2017

NejcZdovc Jul 5, 2017

NejcZdovc commented Jul 5, 2017

bsclifton commented Jul 6, 2017

UrlBarIcon uses now only primitives #9754

UrlBarIcon uses now only primitives #9754

Conversation

NejcZdovc commented Jun 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc Jun 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc commented Jul 5, 2017

bsclifton commented Jul 6, 2017

NejcZdovc commented Jun 28, 2017 •

edited

Loading

bridiver Jun 29, 2017 •

edited

Loading

NejcZdovc Jun 30, 2017 •

edited

Loading