Hide "Trust Token" item from devtools #17118
Conversation
|
|
||
| this.trustTokensTreeElement = new TrustTokensTreeElement(panel); | ||
| - storageTreeElement.appendChild(this.trustTokensTreeElement); | ||
| + //storageTreeElement.appendChild(this.trustTokensTreeElement); |
There was a problem hiding this comment.
@petemill Do you think calling storageTreeElement.removeChild(this.trustTokensTreeElement); would be better than this?
There was a problem hiding this comment.
I can't think why adding and removing is better than just not adding it in the first place. Let me know if you have thought of a reason.
There was a problem hiding this comment.
Oh, NVM then. I thought I've seen something like that in settings page. That's why I asked 😄
There was a problem hiding this comment.
@petemill can we avoid patching here by overriding TrustTokensTreeElement and just making it an empty div or something?
There was a problem hiding this comment.
There is no mechanism to override TrustTokensTreeElement or any piece of TS/JS here without a patch. A more complex one I imagine.
There was a problem hiding this comment.
@sangwoo108 please change this comment to be a single line added above this.trustTokensTreeElement = new TrustTokensTreeElement(panel); with /* and a single line added below storageTreeElement.appendChild(this.trustTokensTreeElement);
We don't want to create TrustTokensTreeElement if we're not going to use it and adding lines is better than modifying them for patches
There was a problem hiding this comment.
Good call! done.
There was a problem hiding this comment.
[2023-03-29T00:54:07.410Z] ../../third_party/devtools-frontend/src/front_end/panels/application/ApplicationPanelSidebar.ts(253,3): error TS2564: Property 'trustTokensTreeElement' has no initializer and is not definitely assigned in the constructor.
Because of this error, added one more line to the patch file.
There was a problem hiding this comment.
Thanks. As discussed, not only does devtools' frontend not use Polymer (or any UI framework?), but it doesn't have the same GN build system, so our existing override methodology is just not going to work. Patching is far far easier, especially since this is only 1 line.
build/commands/lib/util.js
Outdated
| const allPatchStatus = | ||
| chromiumPatchStatus.concat(v8PatchStatus).concat(catapultPatchStatus) | ||
| chromiumPatchStatus.concat(v8PatchStatus).concat(catapultPatchStatus).concat(devtoolsFrontendPatchStatus) |
There was a problem hiding this comment.
Maybe it's time to change this to
const allPatchStatus = [
...v8PatchStatus,
...catapultPatchStatus,
...devtoolsFrontendPatchStatus
]There was a problem hiding this comment.
Cool! Done and added ChromiumPatcheStatus too.
[We disabled the "Trust token" feature](#8039), so we shouldn't reveal this item.
9d81dce to
8f23e9e
Compare
As we're not going to use it, we should add this type.
| databasesListTreeElement: ExpandableApplicationPanelTreeElement; | ||
| cookieListTreeElement: ExpandableApplicationPanelTreeElement; | ||
| - trustTokensTreeElement: TrustTokensTreeElement; | ||
| + trustTokensTreeElement: TrustTokensTreeElement|undefined; |
There was a problem hiding this comment.
oh, was this required because we commented out https://github.com/brave/brave-core/pull/17118/files#diff-23046df64bccd12252ff893d49474e6af4b2831de5d33efd50f3a8c4fc8c11c1R19 ? If so then go back to the original patch because we still end up with a modified line this way
There was a problem hiding this comment.
Sorry, the link seems to be broken(Github are bad at these 😭) . This line was required because we commented out the initialization of this property, which seems to be what you linked. Reverted back to the very original patch - commenting out appendChild()
We disabled the "Trust token" feature, so we shouldn't reveal this item.
Without this patch
With patch
Resolves brave/brave-browser#28394
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_testswikinpm run lint,npm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan:
Manual