New Tab Page: fetch data from JS -> C++ instead of WebUIProperties #2957

petemill · 2019-07-19T06:05:37Z

Summary

Fix brave/brave-browser#5249 (data is more reliable)
Fix brave/brave-browser#3575 (as much as possible, by removing the flash from light -> dark, it now just stays light until the image / gradient comes int)
...and brings in the detection of the poster image loading (and then fading it in) that we had in muon last year!

Updates for New Tab Page

New Tab Page: Serve data via JS -> C++ get APIs

Creates JS -> C++ functions to get data for: Preferences, Stats and PrivateProperties (alternativeSearchEngine). loadTimeData was not a great place for data which can change. Although a DataSource (i.e. strings.js and loadTimeData.data) can be updated, it is complicated and is not normal chromium pattern. Since this data is not required exactly at page creation (since it is not read via static HTML but via React in JS), then we can afford the time it takes to request this data via JS.
Converting to a JS API required refactoring some of the app store / reducer initialization, including creating the concept of 'initial data' which is any data required to render the page. Since we need Preferences to know what to show, and we need Stats since they are shown on the initial page load, then those are contained within.
Includes some minor cleanup with regard to 'API' separation and definition.
Fixes storage being loaded multiple times during redux init (the function is called with an empty state param 3 times, so the state was being loaded from storage 3 times).
Only saves to local storage the state which we will re-use.

New Tab Page WebUI: Graceful page loading

Avoids some flashes and jagged loads, bringing in some features implemented in muon:

Dark background only when dark theme is active.
Fade-in whole page when initial data is fetched.
Fade-in image when load is complete, instead of having the image tear down the screen.

Submitter Checklist:

Test Plan:

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

petemill · 2019-07-19T06:06:14Z

This is feature complete so ready to look at, but just working through some unit tests that need changing up a bit.

petemill · 2019-07-23T23:10:53Z

@imptrx @cezaraugusto this is ready for review - rebased on the recent NTP changes, and modified tests in light of code refactor so that they pass locally.

cezaraugusto

left a few comments

components/brave_new_tab_ui/brave_new_tab.html

cezaraugusto · 2019-07-24T20:33:35Z

components/brave_new_tab_ui/reducers/new_tab_reducer.tsx

+        state.backgroundImage = backgroundAPI.randomBackgroundImage()
+      }
+      console.timeStamp('reducer initial data received')
+      window.setTimeout(function () {


what is the reasoning behind this?

I moved it from the removed case types.NEW_TAB_TOP_SITES_DATA_UPDATED:, which is no longer required as we dispatch a single action for all initial data. I noticed in that removed case, the re-dispatch was not behind a setImmediate, meaning the action could be dispatched synchronously before this action has reduced to state and rendered.

I forgot that webpack does inject a setImmediate polyfill for web, if you feel we should use that.

no need to polyfill but seems valid to add a comment referencing why it is needed. for future reference

I ended up changing all the occurances of this anti-pattern of dispatching an action inside a reducer before the reducer has finished. It can introduce weird bugs where state changes go missing. See discussion at https://stackoverflow.com/questions/36730793/can-i-dispatch-an-action-in-reducer for more information.

I wrapped them in a setImmediate and left a TODO comment to refactor.

cezaraugusto · 2019-07-24T20:35:02Z

components/brave_new_tab_ui/storage.ts

-  state.backgroundImage = backgroundAPI.randomBackgroundImage()
-  state = getLoadTimeData(state)
-  return state
+const getPersistantData = (state: NewTab.State): NewTab.PersistantState => {


s/persistant/persistent?

Oh no. I always get that wrong. Now I need to change it everywhere 🤦‍♀

cezaraugusto · 2019-07-24T20:36:51Z

components/brave_new_tab_ui/reducers/new_tab_reducer.tsx

+    case types.NEW_TAB_PRIVATE_TAB_DATA_UPDATED:
+      state = {
+        ...state,
+        useAlternativePrivateSearchEngine: payload.useAlternativePrivateSearchEngine


nit: you created a const for all other payloads. maybe create one for this one for consistency?

cezaraugusto · 2019-07-24T20:38:25Z

components/brave_new_tab_ui/reducers/new_tab_reducer.tsx

      state = {
        ...state,
        ...preferences
      }
+      if (positivelyChangedShowBackgroundImage) {


this variable name is hard to understand. shouldChangeBackgroundImage maybe?

This variable means "Did the value change from false to true". Your suggestion works 👍

components/definitions/global.d.ts

imptrx

Looks great - just left a few questions 😃
Verified affected areas are working as intended on NTP/NTP Private/Welcome Imports, and original issue is addressed 👍
Noticed we have a lot of console.timeStamps in the code - was wondering if those are just for debugging purposes since it was labeled as a non-standard

components/brave_new_tab_ui/actions/new_tab_actions.ts

components/brave_new_tab_ui/api/topSites/grid.ts

components/brave_new_tab_ui/storage.ts

components/brave_new_tab_ui/components/default/page/index.ts

imptrx · 2019-07-24T23:11:30Z

components/definitions/newTab.d.ts

    isIncognito: boolean
    useAlternativePrivateSearchEngine: boolean
    isTor: boolean
    isQwant: boolean
-    bookmarks: Record<string, Bookmark>
-    stats: Stats
    backgroundImage?: Image


out of context but just realized we didn't propagate Image | undefined down to the actual components using this data - can be done in another PR but quick change if you want to take it 😄

@imptrx Can you give me an example? All the places I can see do do a type check first. Typescript should produce a compiler error if they didn't.

petemill · 2019-07-26T06:35:03Z

All feedback has been addressed @cezaraugusto @imptrx - all changes can be viewed at https://github.com/brave/brave-core/compare/d5a2a7394f3b06860763a3a86bf726923f7220b9..53b3a1f05e56f5b0acb2d327d842d81a61af828c

Fix brave/brave-browser#5249 The previous method of wvh->SetWebUIProperty has issues in that the rvh does not always fire the 'ready' event, especially when navigating 'back'. Using DataSource is a more consistent chromium method for sending properties to the JS context. However, this is usually used for non-changing data. Whilst an UpdateDataSource function does exist, that kind of data is usually pulled from the JS via an explicit call to the C++ WebUI, so we may have to go down that route. The issue with the implementation in this commit is that a page may miss the 'updated' events that fire, for example due to have being navigated away and then back to the page. - Creates JS -> C++ functions to get data for: Preferences, Stats and PrivateProperties (alternativeSearchEngine). loadTimeData is also not a great place for data which can change. Although a DataSource (i.e. strings.js and loadTimeData.data) can be updated, it is complicated and is not normal chromium pattern. Since this data is not required exactly at page creation (since it is not read via static HTML but via React in JS), then we can afford the time it takes to request this data via JS. - Converting to a JS API required refactoring some of the app store / reducer initialization, including creating the concept of 'initial data' which is any data required to render the page. Since we need Preferences to know what to show, and we need Stats since they are shown on the initial page load, then those are contained within. Includes some minor cleanup with regard to 'API' separation and definition. - Fixes storage being loaded multiple times during redux init (the function is called with an empty state param 3 times, so the state was being loaded from storage 3 times). - Only saves to local storage the state which we will be re-used.

Avoids some flashes and jagged loads, bringing in some features implemented in muon: - Dark background only when dark theme is active. - Fade-in whole page when initial data is fetched. - Fade-in image when load is complete, instead of having the image tear down the screen.

promise wasn't being caught, but looks like we don't care

…ucer is finished This is an anti-pattern and could introduce bugs. See discussion at https://stackoverflow.com/questions/36730793/can-i-dispatch-an-action-in-reducer

petemill · 2019-07-26T16:46:47Z

Merging as CI result was that all the builds passed and just received a deps audit warning, which is unrelated to this PR.

kjozwiak · 2019-07-26T16:59:20Z

Adding this and brave/brave-browser#5249 into the 0.70.x - Nightly milestone 👍

…2957) New Tab Page: fetch data from JS -> C++ instead of WebUIProperties

petemill requested review from cezaraugusto and imptrx July 19, 2019 06:05

petemill self-assigned this Jul 19, 2019

petemill force-pushed the fix-5249-ntp-data branch 2 times, most recently from 577e701 to bc522a8 Compare July 23, 2019 23:07

petemill marked this pull request as ready for review July 23, 2019 23:09

petemill force-pushed the fix-5249-ntp-data branch from bc522a8 to 72684d0 Compare July 23, 2019 23:24

cezaraugusto reviewed Jul 24, 2019

View reviewed changes

imptrx reviewed Jul 24, 2019

View reviewed changes

petemill force-pushed the fix-5249-ntp-data branch 2 times, most recently from d5a2a73 to 53b3a1f Compare July 26, 2019 06:32

petemill requested review from cezaraugusto and imptrx July 26, 2019 06:34

petemill added 5 commits July 25, 2019 23:38

Test: JS unit tests mock loadTimeData

9d4ff35

Welcome Page WebUI: Fix lint

5a8efcf

promise wasn't being caught, but looks like we don't care

New Tab Page WebUI: do not dispatch actions inside reducer before red…

e3b2ea7

…ucer is finished This is an anti-pattern and could introduce bugs. See discussion at https://stackoverflow.com/questions/36730793/can-i-dispatch-an-action-in-reducer

petemill force-pushed the fix-5249-ntp-data branch from 53b3a1f to e3b2ea7 Compare July 26, 2019 06:38

cezaraugusto approved these changes Jul 26, 2019

View reviewed changes

petemill merged commit 3e5dc38 into master Jul 26, 2019

petemill deleted the fix-5249-ntp-data branch July 26, 2019 16:47

kjozwiak added this to the 0.70.x - Nightly milestone Jul 26, 2019

mkarolin pushed a commit that referenced this pull request Sep 7, 2019

New Tab Page: fetch data from JS -> C++ instead of WebUIProperties (#…

feddfe9

…2957) New Tab Page: fetch data from JS -> C++ instead of WebUIProperties

petemill mentioned this pull request Sep 9, 2019

C77 on 0.68.x test failure brave/brave-browser#5954

Closed

petemill mentioned this pull request Sep 10, 2019

C77 0.68.x ntp api #3395

Merged

bsclifton mentioned this pull request Sep 10, 2019

Update patches from Chromium 76.0.3809.132 to Chromium 77.0.3865.75 (uplift to 0.68.x) #3385

Merged

4 tasks

btlechowski mentioned this pull request Sep 20, 2019

New tab flashes white when in Light mode brave/brave-browser#3575

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Tab Page: fetch data from JS -> C++ instead of WebUIProperties #2957

New Tab Page: fetch data from JS -> C++ instead of WebUIProperties #2957

petemill commented Jul 19, 2019 •

edited

Loading

petemill commented Jul 19, 2019

petemill commented Jul 23, 2019

cezaraugusto left a comment

cezaraugusto Jul 24, 2019

petemill Jul 24, 2019

cezaraugusto Jul 24, 2019

petemill Jul 26, 2019

cezaraugusto Jul 24, 2019

petemill Jul 24, 2019

cezaraugusto Jul 24, 2019

petemill Jul 26, 2019

cezaraugusto Jul 24, 2019

petemill Jul 24, 2019

imptrx left a comment

imptrx Jul 24, 2019

petemill Jul 26, 2019

petemill commented Jul 26, 2019

petemill commented Jul 26, 2019

kjozwiak commented Jul 26, 2019

New Tab Page: fetch data from JS -> C++ instead of WebUIProperties #2957

New Tab Page: fetch data from JS -> C++ instead of WebUIProperties #2957

Conversation

petemill commented Jul 19, 2019 • edited Loading

Summary

Updates for New Tab Page

New Tab Page: Serve data via JS -> C++ get APIs

New Tab Page WebUI: Graceful page loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

petemill commented Jul 19, 2019

petemill commented Jul 23, 2019

cezaraugusto left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imptrx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemill commented Jul 26, 2019

petemill commented Jul 26, 2019

kjozwiak commented Jul 26, 2019

petemill commented Jul 19, 2019 •

edited

Loading