dont’t call to get site settings outside of mergeProps #9475

bridiver · 2017-06-14T21:39:06Z

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.

Test Plan:

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

diracdeltas · 2017-06-15T00:04:23Z

flash allow-once doesn't seem to work.

go to http://homestarrunner.com/
right click to allow flash once
close the tab then go back to http://homestarrunner.com/
flash is still allowed

same for noscript allow-once

NejcZdovc · 2017-06-25T18:02:49Z

@bridiver can you please rebase? Is scenario by @diracdeltas fixed?

bridiver · 2017-06-29T18:48:37Z

@NejcZdovc @diracdeltas updated to fix that issue

diracdeltas · 2017-06-29T21:27:57Z

app/renderer/components/frame/frame.js

-      appActions.noScriptExceptionsAdded(origin,
-        noScriptExceptions.map(value => value === 0 ? false : value))
+    if (props.noScriptExceptions) {
+      appActions.noScriptExceptionsAdded(props.origin, this.props.noScriptExceptions.filter((value, host) => value !== 0 ? false : value))


i think this line is wrong - the filter function should be a map function like in the original code

also i think it should be props.noScriptExceptions instead of this.props.noScriptExceptions

probably doesn't work because of that. I'll take a look

diracdeltas

noscript 'allow once' still isn't working (scripts are allowed even after the tab is closed and re-opened)

diracdeltas · 2017-06-29T23:22:32Z

app/renderer/components/frame/frame.js

-      appActions.noScriptExceptionsAdded(origin,
-        noScriptExceptions.map(value => value === 0 ? false : value))
+    if (props.noScriptExceptions) {
+      appActions.noScriptExceptionsAdded(props.origin, props.noScriptExceptions.filter((value, host) => value !== 0 ? false : value))


this should still be this.noScriptExceptions.map(value => value === 0 ? false : value)), see c5d2f7b

bridiver · 2017-06-30T00:10:11Z

should be good now

diracdeltas · 2017-06-30T00:13:39Z

@bridiver did you see my comment above ^

bridiver · 2017-06-30T00:14:45Z

no, looks like I mixed it up on the rebase

bridiver · 2017-06-30T00:16:22Z

updated

diracdeltas · 2017-06-30T00:18:54Z

app/renderer/components/frame/frame.js

-      appActions.noScriptExceptionsAdded(origin,
-        noScriptExceptions.map(value => value === 0 ? false : value))
+    if (props.noScriptExceptions) {
+      appActions.noScriptExceptionsAdded(props.origin, props.noScriptExceptions.filter((value, host) => value === 0 ? false : value))


this is still the incorrect version, it should be the version above using map not filter

bridiver · 2017-06-30T00:23:37Z

ok, this time I copy/pasted it so it better be right :)

bsclifton

Do we need tests for this? At minimum, it looks like we might want a unit test for getVisibleOrigin

bridiver · 2017-06-30T03:51:27Z

test added for getVisibleOrigin, but otherwise it's a refactoring so should be covered by existing tests

bsclifton

Thanks for adding the test 😄 ++

dont’t call to get site settings outside of mergeProps

bridiver added perf refactoring/redux-component labels Jun 14, 2017

bridiver added this to the 0.18.x (Developer Channel) milestone Jun 14, 2017

bridiver self-assigned this Jun 14, 2017

bridiver requested review from diracdeltas, bbondy and NejcZdovc June 14, 2017 21:39

bridiver force-pushed the site-settings-merge-props branch from 84c181b to 36e5498 Compare June 19, 2017 23:10

bridiver force-pushed the site-settings-merge-props branch from 36e5498 to 8d9b019 Compare June 29, 2017 18:48

diracdeltas reviewed Jun 29, 2017

View reviewed changes

diracdeltas suggested changes Jun 29, 2017

View reviewed changes

bridiver force-pushed the site-settings-merge-props branch from 8d9b019 to 3ab5898 Compare June 29, 2017 23:09

diracdeltas reviewed Jun 29, 2017

View reviewed changes

bridiver force-pushed the site-settings-merge-props branch from 3ab5898 to 89cf3eb Compare June 30, 2017 00:16

diracdeltas suggested changes Jun 30, 2017

View reviewed changes

bridiver force-pushed the site-settings-merge-props branch from 89cf3eb to 079a5e5 Compare June 30, 2017 00:22

diracdeltas approved these changes Jun 30, 2017

View reviewed changes

bsclifton suggested changes Jun 30, 2017

View reviewed changes

dont’t call to get site settings outside of mergeProps

cc3eedd

bridiver force-pushed the site-settings-merge-props branch from 079a5e5 to cc3eedd Compare June 30, 2017 03:51

NejcZdovc mentioned this pull request Jun 30, 2017

Removes non-primitive props from Frame #9762

Closed

8 tasks

bsclifton added QA/no-qa-needed release-notes/exclude labels Jun 30, 2017

bsclifton approved these changes Jun 30, 2017

View reviewed changes

bsclifton added the automated-tests label Jun 30, 2017

bsclifton merged commit fd26195 into master Jun 30, 2017

bsclifton deleted the site-settings-merge-props branch June 30, 2017 18:45

bsclifton added a commit that referenced this pull request Jun 30, 2017

Merge pull request #9475 from brave/site-settings-merge-props

44ce060

dont’t call to get site settings outside of mergeProps

NejcZdovc mentioned this pull request Jul 10, 2017

Only primitives in Frame #9760

Closed

petemill mentioned this pull request Mar 29, 2018

Enhance performance by attaching only active tabs to a webview (single-webview) #13216

Closed

26 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dont’t call to get site settings outside of mergeProps #9475

dont’t call to get site settings outside of mergeProps #9475

bridiver commented Jun 14, 2017 •

edited by bsclifton

Loading

diracdeltas commented Jun 15, 2017 •

edited

Loading

NejcZdovc commented Jun 25, 2017

bridiver commented Jun 29, 2017

diracdeltas Jun 29, 2017 •

edited

Loading

diracdeltas Jun 29, 2017

bridiver Jun 29, 2017

diracdeltas left a comment

diracdeltas Jun 29, 2017 •

edited

Loading

bridiver commented Jun 30, 2017

diracdeltas commented Jun 30, 2017

bridiver commented Jun 30, 2017

bridiver commented Jun 30, 2017

diracdeltas Jun 30, 2017

bridiver commented Jun 30, 2017

bsclifton left a comment

bridiver commented Jun 30, 2017

bsclifton left a comment

dont’t call to get site settings outside of mergeProps #9475

dont’t call to get site settings outside of mergeProps #9475

Conversation

bridiver commented Jun 14, 2017 • edited by bsclifton Loading

diracdeltas commented Jun 15, 2017 • edited Loading

NejcZdovc commented Jun 25, 2017

bridiver commented Jun 29, 2017

diracdeltas Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

diracdeltas Jun 29, 2017

Choose a reason for hiding this comment

bridiver Jun 29, 2017

Choose a reason for hiding this comment

diracdeltas left a comment

Choose a reason for hiding this comment

diracdeltas Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

bridiver commented Jun 30, 2017

diracdeltas commented Jun 30, 2017

bridiver commented Jun 30, 2017

bridiver commented Jun 30, 2017

diracdeltas Jun 30, 2017

Choose a reason for hiding this comment

bridiver commented Jun 30, 2017

bsclifton left a comment

Choose a reason for hiding this comment

bridiver commented Jun 30, 2017

bsclifton left a comment

Choose a reason for hiding this comment

bridiver commented Jun 14, 2017 •

edited by bsclifton

Loading

diracdeltas commented Jun 15, 2017 •

edited

Loading

diracdeltas Jun 29, 2017 •

edited

Loading

diracdeltas Jun 29, 2017 •

edited

Loading