[<install> Element] Update string and icon for apps already installed [chromium/src : main]
Lia Hiscock (Gerrit)
Lia Hiscock added 1 comment![]( "Open in Gerrit")
Related details
- Lu Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lu Huang (Gerrit)
Lu Huang added 3 comments![]( "Open in Gerrit")
Looks good overall. Please add reviewers.
GURL install_target =
options ? GURL(options->install_url) : last_committed_url_;Do we care if `install_target` is a valid URL or will that be checked later?
mojom::blink::InstallOptionsPtr HTMLInstallElement::PopulateInstallOptions() {Perhaps `[Make|Create]InstallOptions` as this function doesn't take an input.
Related details
- Lia Hiscock
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stanley Hon (Gerrit)
Stanley Hon added 4 comments![]( "Open in Gerrit")
mostly some questions
IN_PROC_BROWSER_TEST_F(InstallElementBrowserTest,let's add a test for the interesting scenario where the button shows 'install' but the app gets uninstalled from under us.
GetDocument()
.GetTaskRunner(TaskType::kInternalDefault)
->PostTask(FROM_HERE, BindOnce(&HTMLInstallElement::UpdateAppearanceTask,
WrapWeakPersistent(this)));mainly curious - is it required we use the task runner to queue a task to make a mojo call, then proceed to wait to be called back (not on any task queue)?
is it okay to just make the mojo call here (i.e. `WebInstallService()->IsInstalled()`) and not use the task runner?
// Can be nullptr in unit tests and after Shutdown().hmm... the ergonomics of using this is odd, we use `WebInstallService()` as kind of a "if it's not bound, bind it, then return it' - that's okay. But we also have to constantly check `is_bound()` on the return value but that pattern seems common across other HeapMojoRemotes too.
the main question I have is, I can't make sense of ExecutionContext's lifetime in relation to `HtmlInstallElement`. If ExecutionContext is gone, we can't do anything (aka rebind the remote). So, what should we do when ExecutionContext is gone?
maybe someone with more domain expertise here can comment on how much of a concern this is.
Lia Hiscock (Gerrit)
Lia Hiscock added 2 comments![]( "Open in Gerrit")
GURL install_target =
options ? GURL(options->install_url) : last_committed_url_;Do we care if `install_target` is a valid URL or will that be checked later?
this is enforced on the blink side by the element. do you think a `CHECK` is appropriate here then? i can also rename GetInstallOptions to indicate that it performs some validations?
```
mojom::blink::InstallOptionsPtr HTMLInstallElement::GetInstallOptions() {
mojom::blink::InstallOptionsPtr options;
KURL install_url = KURL(InstallUrl());
if (install_url.IsValid()) {
options = mojom::blink::InstallOptions::New();
options->install_url = install_url;
```
mojom::blink::InstallOptionsPtr HTMLInstallElement::PopulateInstallOptions() {Perhaps `[Make|Create]InstallOptions` as this function doesn't take an input.
Makes sense! I went with GetInstallOptions since we're just grabbing them from the attributes
Related details
- Lu Huang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Murphy (Gerrit)
Daniel Murphy added 1 comment![]( "Open in Gerrit")
// We'll update the text ourselves rather than invoking
// `HTMLPermissionElement::UpdateAppearance()` given the logic here. Like
// `HTMLGeolocationElement::UpdateAppearance()`, we'll punt the inner text
// change out to a task on the default task queue.```
// We need `PostTask` here because setInnerText hits a DCHECK.
```
@tun...@chromium.org Can you help us make a better comment here? WHY does this DCHECK? we should already be on the kInternalDefault queue if the mojo pipe is bound on that queue?
Related details
- Lia Hiscock
- Lu Huang
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lia Hiscock (Gerrit)
Lia Hiscock added 1 comment![]( "Open in Gerrit")
GetDocument()
.GetTaskRunner(TaskType::kInternalDefault)
->PostTask(FROM_HERE, BindOnce(&HTMLInstallElement::UpdateAppearanceTask,
WrapWeakPersistent(this)));mainly curious - is it required we use the task runner to queue a task to make a mojo call, then proceed to wait to be called back (not on any task queue)?
is it okay to just make the mojo call here (i.e. `WebInstallService()->IsInstalled()`) and not use the task runner?
hi again @tun...@chromium.org! Mike referred us to you for SME help in this area. I'm new to working with JS and the task queue, and would appreciate any insight you have. Thanks!
Related details
- Lu Huang
- Thomas Nguyen
Lia Hiscock (Gerrit)
Lia Hiscock added 1 comment![]( "Open in Gerrit")
GURL install_target =
options ? GURL(options->install_url) : last_committed_url_;Lia HiscockDo we care if `install_target` is a valid URL or will that be checked later?
this is enforced on the blink side by the element. do you think a `CHECK` is appropriate here then? i can also rename GetInstallOptions to indicate that it performs some validations?
```
mojom::blink::InstallOptionsPtr HTMLInstallElement::GetInstallOptions() {
mojom::blink::InstallOptionsPtr options;KURL install_url = KURL(InstallUrl());
if (install_url.IsValid()) {
options = mojom::blink::InstallOptions::New();
options->install_url = install_url;
```
actually, rethinking this, since we don't trust data from blink, let me add an if check for this here :) LMK if you want more checks
Related details
- Lu Huang
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lia Hiscock (Gerrit)
Lia Hiscock added 1 comment![]( "Open in Gerrit")
Hi folks! While we sort out the task runner questions, thought I'd get y'all added to start looking at the rest.
Related details
- Chromium IPC Reviews
- Daniel Murphy
- Lu Huang
- Mike West
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: mk...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): mk...@chromium.org
Note: IPC gwsq added no new reviewers; existing reviewers satisfied requirements!
Reviewer source(s):
mk...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Daniel Murphy
- Lu Huang
- Mike West
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Murphy (Gerrit)
Daniel Murphy added 4 comments![]( "Open in Gerrit")
std::optional<webapps::AppId> IsAppInstalled(nit: Have this take a `WebAppProvider&` instead of a `Profile*`
GURL install_target =
options ? GURL(options->install_url) : last_committed_url_;```suggestion
GURL install_target;
std::optional<GURL> manifest_id;
if (options) {
install_target = GURL(options->install_url);
manifest_id = options->manifest_id;
} else {
install_target = last_committed_url_;
}
```
if (!install_target.is_valid() || !install_target.SchemeIsHTTPOrHTTPS()) {fyi: If the renderer is supposed to enforce this, then we can use the ReportBadMessage to report a misbehaving renderer, which crashes the renderer (e.g. it must be compromised).
returning false is also ok - just fyi.
}
std::optional<GURL> manifest_id = std::nullopt;
if (options && options->manifest_id.has_value()) {
manifest_id = options->manifest_id.value();
}
```suggestion
}
```
Related details
- Lia Hiscock
- Mike West
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lia Hiscock (Gerrit)
Lia Hiscock added 4 comments![]( "Open in Gerrit")
nit: Have this take a `WebAppProvider&` instead of a `Profile*`
Done
GURL install_target =
options ? GURL(options->install_url) : last_committed_url_;```suggestion
GURL install_target;
std::optional<GURL> manifest_id;
if (options) {
install_target = GURL(options->install_url);
manifest_id = options->manifest_id;
} else {
install_target = last_committed_url_;
}
```
Done - with a slight modification, as `options->manifest_id` is an optional field
if (!install_target.is_valid() || !install_target.SchemeIsHTTPOrHTTPS()) {fyi: If the renderer is supposed to enforce this, then we can use the ReportBadMessage to report a misbehaving renderer, which crashes the renderer (e.g. it must be compromised).
returning false is also ok - just fyi.
Oh interesting! Yeah on the renderer side we check `install_target.is_valid()` after we grab the attribute's value [HTMLInstallElement::GetInstallOptions](https://chromium-review.googlesource.com/c/chromium/src/+/7302388/22/third_party/blink/renderer/core/html/html_install_element.cc#155), but we do not check for HTTP/S.
If there's no strong preference, I'll probably just leave it as is, but good to know that ReportBadMessage is an option!
std::optional<GURL> manifest_id = std::nullopt;
if (options && options->manifest_id.has_value()) {
manifest_id = options->manifest_id.value();
}
- Daniel Murphy
- Mike West
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Murphy (Gerrit)
Daniel Murphy voted and added 3 comments![]( "Open in Gerrit")
Votes added by Daniel Murphy
| Code-Review | +1 |
3 comments
EXPECT_EQ(provider().registrar_unsafe().GetAppIds().size(), 1u);in general, I recommend you use AppMatches instead of checking registrar size. Registrar size seems fragile with things like preinstall, and even GetAppIds isn't a great API method and should take a filter too.
So - if possible, please generate the app_id that should be used here (GetURL from the server of the manifestid path, then call GenerateAppIdFromManifestId), and check AppMatches with that.
if (options->manifest_id.has_value()) {
manifest_id = options->manifest_id;
}```suggestion
manifest_id = options->manifest_id;
```
Both fields are optional, right? assigning from one optional to the other works.
Related details
- Lia Hiscock
- Mike West
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Nguyen (Gerrit)
Thomas Nguyen added 2 comments![]( "Open in Gerrit")
bool is_launch_ = false;Ditto, please move it to HTMLInstallElement if possible.
void SetLaunch(bool is_launch);can we move this to HTMLInstallElement? We only want to keep the common members in base class (SetPreciseLocation will be removed soon)
Related details
- Lia Hiscock
- Mike West
Lia Hiscock (Gerrit)
Lia Hiscock added 4 comments![]( "Open in Gerrit")
let's add a test for the interesting scenario where the button shows 'install' but the app gets uninstalled from under us.
Done - `InstallWithUrl_AlreadyInstalledThenUninstalled`
EXPECT_EQ(provider().registrar_unsafe().GetAppIds().size(), 1u);in general, I recommend you use AppMatches instead of checking registrar size. Registrar size seems fragile with things like preinstall, and even GetAppIds isn't a great API method and should take a filter too.
So - if possible, please generate the app_id that should be used here (GetURL from the server of the manifestid path, then call GenerateAppIdFromManifestId), and check AppMatches with that.
Done - replaced usages in this file
if (options->manifest_id.has_value()) {
manifest_id = options->manifest_id;
}```suggestion
manifest_id = options->manifest_id;
```Both fields are optional, right? assigning from one optional to the other works.
That's my bad. Yes we can definitely just directly assign 😂
hmm... the ergonomics of using this is odd, we use `WebInstallService()` as kind of a "if it's not bound, bind it, then return it' - that's okay. But we also have to constantly check `is_bound()` on the return value but that pattern seems common across other HeapMojoRemotes too.
the main question I have is, I can't make sense of ExecutionContext's lifetime in relation to `HtmlInstallElement`. If ExecutionContext is gone, we can't do anything (aka rebind the remote). So, what should we do when ExecutionContext is gone?
maybe someone with more domain expertise here can comment on how much of a concern this is.
Done - confirmed in last install sync that Mike/Dan are good with us doing nothing if the execution context is gone. I've added some comments for future us 😊
Related details
- Daniel Murphy
- Mike West
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Nguyen (Gerrit)
Thomas Nguyen added 2 comments![]( "Open in Gerrit")
// We'll update the text ourselves rather than invoking
// `HTMLPermissionElement::UpdateAppearance()` given the logic here. Like
// `HTMLGeolocationElement::UpdateAppearance()`, we'll punt the inner text
// change out to a task on the default task queue.```
// We need `PostTask` here because setInnerText hits a DCHECK.
```@tun...@chromium.org Can you help us make a better comment here? WHY does this DCHECK? we should already be on the kInternalDefault queue if the mojo pipe is bound on that queue?
During the layout phase, the DCHECK hits by setInnerText and another checks hit within event dispatching, `!EventDispatchForbiddenScope::IsEventDispatchForbidden().`
Because these issues occur inconsistently, I chose to defer the task to the queue. While I did not preserve the stack trace, my previous notes shows the situation is similar to https://g-issues.chromium.org/issues/40901745.
GetDocument()
.GetTaskRunner(TaskType::kInternalDefault)
->PostTask(FROM_HERE, BindOnce(&HTMLInstallElement::UpdateAppearanceTask,
WrapWeakPersistent(this)));Lia Hiscockmainly curious - is it required we use the task runner to queue a task to make a mojo call, then proceed to wait to be called back (not on any task queue)?
is it okay to just make the mojo call here (i.e. `WebInstallService()->IsInstalled()`) and not use the task runner?
hi again @tun...@chromium.org! Mike referred us to you for SME help in this area. I'm new to working with JS and the task queue, and would appreciate any insight you have. Thanks!
Since the delayed task is used for UI update calls, calling WebInstallService()->IsInstalled() within this particular task doesn't seem necessary (unless it's directly UI relevant)
Related details
- Daniel Murphy
- Lia Hiscock
- Mike West
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Murphy (Gerrit)
Daniel Murphy added 3 comments![]( "Open in Gerrit")
GenerateAppIdFromManifestId(https_server()->GetURL("/some_id"));this seems incorrect? The id is set here:
https://source.chromium.org/chromium/chromium/src/+/main:chrome/test/data/web_apps/install_element/manifest.json
and it should be `/my_id` instead of `/some_id`?
You could maybe pull out some things:
```
static constexpr std::string_view kInstallElementPageStartUrl = /web_apps/install_element/index.html";
static constexpr std::string_view kInstallElementPageId = "/my_id";
```
// We'll update the text ourselves rather than invoking
// `HTMLPermissionElement::UpdateAppearance()` given the logic here. Like
// `HTMLGeolocationElement::UpdateAppearance()`, we'll punt the inner text
// change out to a task on the default task queue.Thomas Nguyen```
// We need `PostTask` here because setInnerText hits a DCHECK.
```@tun...@chromium.org Can you help us make a better comment here? WHY does this DCHECK? we should already be on the kInternalDefault queue if the mojo pipe is bound on that queue?
During the layout phase, the DCHECK hits by setInnerText and another checks hit within event dispatching, `!EventDispatchForbiddenScope::IsEventDispatchForbidden().`
Because these issues occur inconsistently, I chose to defer the task to the queue. While I did not preserve the stack trace, my previous notes shows the situation is similar to https://g-issues.chromium.org/issues/40901745.
A few thought:
- If we are worried about mojo messages on this pipe being dispatched during layout, then the PostTask actually shouldn't happen here, it should happen after `OnIsInstalledResult`, as we get the reply from that to then call `setInnerText`.
- I'm suspicious about this, as as far as I can tell, the mojo pipe is already bound on kInternalDefault - but maybe it's not. But - why does kInternalDefault not conflict with layout? I would imagine that it could also be run during layout. But - if it worked to remove the crash, then great.
- Another idea would be to make sure the mojo pipe is bound on this task runner to begin with, so that ALL calls here will be dispatched at a time when the layout isn't happening. But that can be a follow-up if more research is done.
- The commend should probably be, if all of this is right, next to the the PostTask in OnIsInstalledResult:
```
// This is posted as a task, as similar code in
// `HTMLGeolocationElement::UpdateAppearance` would crash due to DCHECKs being
// hit for calling setInnerText during layout.
// TODO(https://crbug.com/<number>): If possible, bind the mojo pipe to a task
// runner that cannot be called during layout, to avoid this.
```
And then file a bug and maybe assign to Thomas to follow up, which will simplify both this and the geolocation element.
GetDocument()
.GetTaskRunner(TaskType::kInternalDefault)
->PostTask(FROM_HERE, BindOnce(&HTMLInstallElement::UpdateAppearanceTask,
WrapWeakPersistent(this)));Lia Hiscockmainly curious - is it required we use the task runner to queue a task to make a mojo call, then proceed to wait to be called back (not on any task queue)?
is it okay to just make the mojo call here (i.e. `WebInstallService()->IsInstalled()`) and not use the task runner?
Thomas Nguyenhi again @tun...@chromium.org! Mike referred us to you for SME help in this area. I'm new to working with JS and the task queue, and would appreciate any insight you have. Thanks!
Since the delayed task is used for UI update calls, calling WebInstallService()->IsInstalled() within this particular task doesn't seem necessary (unless it's directly UI relevant)
- Lia Hiscock
- Mike West
Thomas Nguyen (Gerrit)
Thomas Nguyen added 1 comment![]( "Open in Gerrit")
GetDocument()
.GetTaskRunner(TaskType::kInternalDefault)
->PostTask(FROM_HERE, BindOnce(&HTMLInstallElement::UpdateAppearanceTask,
WrapWeakPersistent(this)));Lia Hiscockmainly curious - is it required we use the task runner to queue a task to make a mojo call, then proceed to wait to be called back (not on any task queue)?
is it okay to just make the mojo call here (i.e. `WebInstallService()->IsInstalled()`) and not use the task runner?
Thomas Nguyenhi again @tun...@chromium.org! Mike referred us to you for SME help in this area. I'm new to working with JS and the task queue, and would appreciate any insight you have. Thanks!
Daniel MurphySince the delayed task is used for UI update calls, calling WebInstallService()->IsInstalled() within this particular task doesn't seem necessary (unless it's directly UI relevant)
It's UI relevant - it changes what the UI shows.
If the text update happens after the OnIsInstalledResult (already delayed) I think we will be fine. The previous crash only happens if we call setInnerText directly (with a mix of updating granted pseudo element)
Lia Hiscock (Gerrit)
Lia Hiscock added 3 comments![]( "Open in Gerrit")
GenerateAppIdFromManifestId(https_server()->GetURL("/some_id"));this seems incorrect? The id is set here:
https://source.chromium.org/chromium/chromium/src/+/main:chrome/test/data/web_apps/install_element/manifest.jsonand it should be `/my_id` instead of `/some_id`?
You could maybe pull out some things:
```
static constexpr std::string_view kInstallElementPageStartUrl = /web_apps/install_element/index.html";
static constexpr std::string_view kInstallElementPageId = "/my_id";
```
I've been going back and forth on the clearest approach here re. ids and scopes, considering we have 3 different test pages.
You're right that in main `install_element.html` has `my_id`, but in this CL I changed to `some_id` for consistency.
I pulled out all 3 test sites' info into constexprs, LMK if it's still unclear.
// We'll update the text ourselves rather than invoking
// `HTMLPermissionElement::UpdateAppearance()` given the logic here. Like
// `HTMLGeolocationElement::UpdateAppearance()`, we'll punt the inner text
// change out to a task on the default task queue.Thomas Nguyen```
// We need `PostTask` here because setInnerText hits a DCHECK.
```@tun...@chromium.org Can you help us make a better comment here? WHY does this DCHECK? we should already be on the kInternalDefault queue if the mojo pipe is bound on that queue?
Daniel MurphyDuring the layout phase, the DCHECK hits by setInnerText and another checks hit within event dispatching, `!EventDispatchForbiddenScope::IsEventDispatchForbidden().`
Because these issues occur inconsistently, I chose to defer the task to the queue. While I did not preserve the stack trace, my previous notes shows the situation is similar to https://g-issues.chromium.org/issues/40901745.
A few thought:
- If we are worried about mojo messages on this pipe being dispatched during layout, then the PostTask actually shouldn't happen here, it should happen after `OnIsInstalledResult`, as we get the reply from that to then call `setInnerText`.
- I'm suspicious about this, as as far as I can tell, the mojo pipe is already bound on kInternalDefault - but maybe it's not. But - why does kInternalDefault not conflict with layout? I would imagine that it could also be run during layout. But - if it worked to remove the crash, then great.
- Another idea would be to make sure the mojo pipe is bound on this task runner to begin with, so that ALL calls here will be dispatched at a time when the layout isn't happening. But that can be a follow-up if more research is done.
- The commend should probably be, if all of this is right, next to the the PostTask in OnIsInstalledResult:```
// This is posted as a task, as similar code in
// `HTMLGeolocationElement::UpdateAppearance` would crash due to DCHECKs being
// hit for calling setInnerText during layout.
// TODO(https://crbug.com/<number>): If possible, bind the mojo pipe to a task
// runner that cannot be called during layout, to avoid this.
```And then file a bug and maybe assign to Thomas to follow up, which will simplify both this and the geolocation element.
Thanks for weighing in here Dan. I've moved the PostTask into `OnIsInstalledResult`, added the suggested comment, and opened this bug -
https://issues.chromium.org/issues/477974745
I'm going to resolve the other comment thread to consolidate further discussion.
GetDocument()
.GetTaskRunner(TaskType::kInternalDefault)
->PostTask(FROM_HERE, BindOnce(&HTMLInstallElement::UpdateAppearanceTask,
WrapWeakPersistent(this)));Lia Hiscockmainly curious - is it required we use the task runner to queue a task to make a mojo call, then proceed to wait to be called back (not on any task queue)?
is it okay to just make the mojo call here (i.e. `WebInstallService()->IsInstalled()`) and not use the task runner?
Thomas Nguyenhi again @tun...@chromium.org! Mike referred us to you for SME help in this area. I'm new to working with JS and the task queue, and would appreciate any insight you have. Thanks!
Daniel MurphySince the delayed task is used for UI update calls, calling WebInstallService()->IsInstalled() within this particular task doesn't seem necessary (unless it's directly UI relevant)
Thomas NguyenIt's UI relevant - it changes what the UI shows.
If the text update happens after the OnIsInstalledResult (already delayed) I think we will be fine. The previous crash only happens if we call setInnerText directly (with a mix of updating granted pseudo element)
Acknowledged - moving discussion into the other comment threads
Related details
- Daniel Murphy
- Mike West
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Murphy (Gerrit)
Daniel Murphy voted Code-Review+1![]( "Open in Gerrit")
- Lia Hiscock
- Mike West
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lia Hiscock (Gerrit)
Lia Hiscock added 3 comments![]( "Open in Gerrit")
Moved is_launch_ into HTML install element! If this wasn't what/how you were envisioning it, more specific guidance would be great :)
Ditto, please move it to HTMLInstallElement if possible.
Done
can we move this to HTMLInstallElement? We only want to keep the common members in base class (SetPreciseLocation will be removed soon)
- Mike West
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lia Hiscock (Gerrit)
Lia Hiscock added 1 comment![]( "Open in Gerrit")
@tun...@google.com LMK if you have any other thoughts here too
Related details
- Mike West
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike West (Gerrit)
Mike West voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mike West
| Code-Review | +1 |
1 comment
//third_party/blink and mojo LGTM. I think we can defer the discussion of the dcheck to a subsequent CL that could address it more broadly if we decide that's necessary or helpful.
Related details
- Daniel Murphy
- Lia Hiscock
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lia Hiscock (Gerrit)
Lia Hiscock added 2 comments![]( "Open in Gerrit")
//third_party/blink and mojo LGTM. I think we can defer the discussion of the dcheck to a subsequent CL that could address it more broadly if we decide that's necessary or helpful.
SGTM. I've already opened the followup bug (and Dan did +1, but it got reset when I uploaded the sha), so I'll go ahead and resolve the open comment thread.
// We'll update the text ourselves rather than invoking
// `HTMLPermissionElement::UpdateAppearance()` given the logic here. Like
// `HTMLGeolocationElement::UpdateAppearance()`, we'll punt the inner text
// change out to a task on the default task queue.Thomas Nguyen```
// We need `PostTask` here because setInnerText hits a DCHECK.
```@tun...@chromium.org Can you help us make a better comment here? WHY does this DCHECK? we should already be on the kInternalDefault queue if the mojo pipe is bound on that queue?
Daniel MurphyDuring the layout phase, the DCHECK hits by setInnerText and another checks hit within event dispatching, `!EventDispatchForbiddenScope::IsEventDispatchForbidden().`
Because these issues occur inconsistently, I chose to defer the task to the queue. While I did not preserve the stack trace, my previous notes shows the situation is similar to https://g-issues.chromium.org/issues/40901745.
Lia HiscockA few thought:
- If we are worried about mojo messages on this pipe being dispatched during layout, then the PostTask actually shouldn't happen here, it should happen after `OnIsInstalledResult`, as we get the reply from that to then call `setInnerText`.
- I'm suspicious about this, as as far as I can tell, the mojo pipe is already bound on kInternalDefault - but maybe it's not. But - why does kInternalDefault not conflict with layout? I would imagine that it could also be run during layout. But - if it worked to remove the crash, then great.
- Another idea would be to make sure the mojo pipe is bound on this task runner to begin with, so that ALL calls here will be dispatched at a time when the layout isn't happening. But that can be a follow-up if more research is done.
- The commend should probably be, if all of this is right, next to the the PostTask in OnIsInstalledResult:```
// This is posted as a task, as similar code in
// `HTMLGeolocationElement::UpdateAppearance` would crash due to DCHECKs being
// hit for calling setInnerText during layout.
// TODO(https://crbug.com/<number>): If possible, bind the mojo pipe to a task
// runner that cannot be called during layout, to avoid this.
```And then file a bug and maybe assign to Thomas to follow up, which will simplify both this and the geolocation element.
Lia HiscockThanks for weighing in here Dan. I've moved the PostTask into `OnIsInstalledResult`, added the suggested comment, and opened this bug -
https://issues.chromium.org/issues/477974745I'm going to resolve the other comment thread to consolidate further discussion.
@tun...@google.com LMK if you have any other thoughts here too
- Daniel Murphy
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
Lia Hiscock (Gerrit)
Lia Hiscock voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Lia Hiscock (Gerrit)
Lia Hiscock voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Daniel Murphy
- Thomas Nguyen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
36 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/web_applications/install_element_browsertest.cc
Insertions: 2, Deletions: 2.
The diff is too large to show. Please review the diff.
```
Change information
[<install> Element] Update string and icon for apps already installed
* Implement `IsInstalled` for the WebInstallService interface
* HTMLInstallElement now stores show_as_launch_ status
* HTMLInstallElement now has its own icon updating logic
* Add IDS and SVG for 'launch' button string and icon
* Misc test improvements - use AppMatches, refactor test site paths, etc
- Feature flag: InstallElement
- Explainer: https://github.com/WICG/install-element
- Dev spec:
https://docs.google.com/document/d/1rGvLhD4SR8Y9M1wVmqgyesPNkbZGU7HOqlttjEFJ5Vo/edit?tab=t.tmx19oox759l#heading=h.j3tt49hqiuck
- Screenshot:
https://drive.google.com/file/d/1yEtGZpvA_bdBiVcQkHu_zrbxZckNTL4S/view?usp=drive_link
- UX review doc:
https://docs.google.com/document/d/1rGvLhD4SR8Y9M1wVmqgyesPNkbZGU7HOqlttjEFJ5Vo/edit?tab=t.w9u0and0mnhh#heading=h.6ldxrb8j9m13
- M chrome/browser/web_applications/install_element_browsertest.cc
- M chrome/browser/web_applications/web_install_service_impl.cc
- M chrome/browser/web_applications/web_install_service_impl.h
- M chrome/test/data/web_apps/custom_id/manifest.json
- M chrome/test/data/web_apps/install_element/manifest.json
- M third_party/blink/public/blink_resources.grd
- M third_party/blink/public/mojom/web_install/web_install.mojom
- M third_party/blink/public/strings/permission_element_strings.grd
- A third_party/blink/public/strings/permission_element_strings_grd/IDS_PERMISSION_REQUEST_LAUNCH.png.sha1
- M third_party/blink/renderer/core/html/html_install_element.cc
- M third_party/blink/renderer/core/html/html_install_element.h
- M third_party/blink/renderer/core/html/html_install_element_test.cc
- M third_party/blink/renderer/core/html/html_permission_element.cc
- M third_party/blink/renderer/core/html/html_permission_element.h
- M third_party/blink/renderer/core/html/html_permission_icon_element.cc
- M third_party/blink/renderer/core/html/html_permission_icon_element.h
- A third_party/blink/renderer/core/html/resources/images/permission_icon_install_launch.svg
Code-Review: +1 by Mike West
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |