Add mojo typemap for ts Url [chromium/src : main]
Fred Shih (Gerrit)
Fred Shih voted and added 2 comments![]( "Open in Gerrit")
Votes added by Fred Shih
| Commit-Queue | +1 |
2 comments
Fred ShihNit: Add a small sentence explaining that this updates Mojo's `Url` class throughout the codebase to just `string` ?
Done
// found in the LICENSE file.Fred ShihNit: Blank line after licensce?
Done
Related details
- Michael Cui
- Roman Arora
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. |
Michael Cui (Gerrit)
Michael Cui added 11 comments![]( "Open in Gerrit")
I don't have much bandwidth to review, so this is just a quick skim - sorry!
Unfortunately Polymer - still used on CrOS - does not have type checking in templates. I think you will need to assume that [Beyoncé rule](https://abseil.io/resources/swe-book/html/ch11.html#the_beyonceacutesemicolon_rule) applies - i.e. you are free to break anyone which uses `.url` in a Polymer template if they don't have a test for it.
It might be worth doing some due diligence and pre-emptively looking out for _any_ usage of `.url` in Polymer templates, in case some aren't covered by tests.
photoUrl: session.teacher.photoUrl ? session.teacher.photoUrl : undefined,Assuming that `session.teacher.photoUrl` is `string | null` here, this can be replaced with `session.teacher.photoUrl ?? undefined` (here and below).
if (!leftBudImageUrl || !rightBudImageUrl || !caseImageUrl) {
return false;
}This can be removed now (as it's equivalent to the return below).
export function isImageDataUrl(maybeDataUrl: unknown): maybeDataUrl is Url {
return typeof maybeDataUrl === 'string' &&
(maybeDataUrl.startsWith('data:image/png;base64') ||
maybeDataUrl.startsWith('data:image/jpeg;base64'));
}(pre-existing issue)
This is quite scary as type predicates must be a two-way implication: "`obj` is T iff `typeAssertion(obj)`", but isn't the case. See https://effectivetypescript.com/2024/02/27/type-guards/#What-if-you-return-false.
We should probably add a TODO here to address this, as it's outside the scope of this CL.
// TODO(b/279132899): Figure out why `url` is an empty object when it shouldHave you looked at this bug? If this bug isn't fixed, will this CL break in the cases where `url` was an empty object?
const urlList: Url[] = urls;
const {productInfos} =
await this.shoppingApi_.getProductInfoForUrls(urlList);optional: This can be simplified:
```suggestion
const {productInfos} =
await this.shoppingApi_.getProductInfoForUrls(urlList);
```
protected accessor tableUrl_: Url = '';Leave out type annotations for trivially inferred types: variables or parameters initialized to a `string`, `number`, `boolean`, `RegExp` literal or `new` expression.
https://google.github.io/styleguide/tsguide.html#type-inference
I assume this also applies to `accessor`s too.
if (url === null) {
return undefined;
}
return url;This can now be simplified to `url ?? undefined`.
return this.theme_ && this.theme_.backgroundImageAttributionUrl ?
this.theme_.backgroundImageAttributionUrl :
'';This can be simplified to match the surrounding methods:
```suggestion
return this.theme_ && this.theme_.backgroundImageAttributionUrl || '';
```
accessor url: Url = '';Leave out type annotations for trivially inferred types: variables or parameters initialized to a `string`, `number`, `boolean`, `RegExp` literal or `new` expression.
https://google.github.io/styleguide/tsguide.html#type-inference
I assume this also applies to `accessor`s too.
// The wallpaper controller is expected to impose max resolution.I think we should probably undo this change, and just adjust line 341 above to match the other tests in this file.
Related details
- Fred Shih
- Roman Arora
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. |
Fred Shih (Gerrit)
Fred Shih added 12 comments![]( "Open in Gerrit")
I don't have much bandwidth to review, so this is just a quick skim - sorry!
Unfortunately Polymer - still used on CrOS - does not have type checking in templates. I think you will need to assume that [Beyoncé rule](https://abseil.io/resources/swe-book/html/ch11.html#the_beyonceacutesemicolon_rule) applies - i.e. you are free to break anyone which uses `.url` in a Polymer template if they don't have a test for it.
It might be worth doing some due diligence and pre-emptively looking out for _any_ usage of `.url` in Polymer templates, in case some aren't covered by tests.
I did do a grep across all the .html files and tried to catch as many as I could. Obviously this doesn't catch the cases when they don't use the field accessors, but I can't think of any feasible way to catch them (and gemini couldn't either..).
I did play around with the emulator as much as I could to see if there are any regressions. But given the size of this CL, it's hard to know for sure...
photoUrl: session.teacher.photoUrl ? session.teacher.photoUrl : undefined,Assuming that `session.teacher.photoUrl` is `string | null` here, this can be replaced with `session.teacher.photoUrl ?? undefined` (here and below).
done, I did not update the rest of the file though. Some day we will fix the null vs undefined for mojo...
if (!leftBudImageUrl || !rightBudImageUrl || !caseImageUrl) {
return false;
}This can be removed now (as it's equivalent to the return below).
Done
export function isImageDataUrl(maybeDataUrl: unknown): maybeDataUrl is Url {
return typeof maybeDataUrl === 'string' &&
(maybeDataUrl.startsWith('data:image/png;base64') ||
maybeDataUrl.startsWith('data:image/jpeg;base64'));
}(pre-existing issue)
This is quite scary as type predicates must be a two-way implication: "`obj` is T iff `typeAssertion(obj)`", but isn't the case. See https://effectivetypescript.com/2024/02/27/type-guards/#What-if-you-return-false.
We should probably add a TODO here to address this, as it's outside the scope of this CL.
Added a TODO.
// TODO(b/279132899): Figure out why `url` is an empty object when it shouldHave you looked at this bug? If this bug isn't fixed, will this CL break in the cases where `url` was an empty object?
I don't think this is a mojo issue. This looks like it's a problem in the help_app_ui page itself. At some point they're passing an object type to themselves, which is handled by the if statement. The second clause checks if it's not a string type and returns an empty string if that's the case.
return {...value, url: value.url + '=s512'};Fred ShihI have no idea why this part is needed..
Acknowledged
const urlList: Url[] = urls;
const {productInfos} =
await this.shoppingApi_.getProductInfoForUrls(urlList);optional: This can be simplified:
```suggestion
const {productInfos} =
await this.shoppingApi_.getProductInfoForUrls(urlList);
```
Done
Leave out type annotations for trivially inferred types: variables or parameters initialized to a `string`, `number`, `boolean`, `RegExp` literal or `new` expression.
https://google.github.io/styleguide/tsguide.html#type-inference
I assume this also applies to `accessor`s too.
Done
This can now be simplified to `url ?? undefined`.
this is not equivalent, '' evaluates to false. url ?? undefined would change the empty string return to an undefined return.
It probably won't make a diff in the implementation, but I'd like to avoid unnecessary behavioural changes in this monster CL :p
return this.theme_ && this.theme_.backgroundImageAttributionUrl ?
this.theme_.backgroundImageAttributionUrl :
'';This can be simplified to match the surrounding methods:
```suggestion
return this.theme_ && this.theme_.backgroundImageAttributionUrl || '';
```
Done
Leave out type annotations for trivially inferred types: variables or parameters initialized to a `string`, `number`, `boolean`, `RegExp` literal or `new` expression.
https://google.github.io/styleguide/tsguide.html#type-inference
I assume this also applies to `accessor`s too.
Done
// The wallpaper controller is expected to impose max resolution.I think we should probably undo this change, and just adjust line 341 above to match the other tests in this file.
The previous setup no longer works and is somewhat brittle. It used to work because the forEach photo was an object, so it was passed by reference. Therefore url += '=s512' would mutate the existing object. Now that photos is a string, it is copied in the iteration, so photo += '=s512' no longer mutates the existing photos object.
It's just a bad pattern to hold on to a large object that is mutated throughout the test. It took me a while to figure out why this test broke :\
Related details
- Demetrios Papadopoulos
- Michael Cui
- Roman Arora
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. |
Michael Cui (Gerrit)
Michael Cui voted and added 3 comments![]( "Open in Gerrit")
Votes added by Michael Cui
| Code-Review | +1 |
Fred ShihI don't have much bandwidth to review, so this is just a quick skim - sorry!
Unfortunately Polymer - still used on CrOS - does not have type checking in templates. I think you will need to assume that [Beyoncé rule](https://abseil.io/resources/swe-book/html/ch11.html#the_beyonceacutesemicolon_rule) applies - i.e. you are free to break anyone which uses `.url` in a Polymer template if they don't have a test for it.
It might be worth doing some due diligence and pre-emptively looking out for _any_ usage of `.url` in Polymer templates, in case some aren't covered by tests.
I did do a grep across all the .html files and tried to catch as many as I could. Obviously this doesn't catch the cases when they don't use the field accessors, but I can't think of any feasible way to catch them (and gemini couldn't either..).
I did play around with the emulator as much as I could to see if there are any regressions. But given the size of this CL, it's hard to know for sure...
Acknowledged
if (url === null) {
return undefined;
}
return url;Fred ShihThis can now be simplified to `url ?? undefined`.
this is not equivalent, '' evaluates to false. url ?? undefined would change the empty string return to an undefined return.
It probably won't make a diff in the implementation, but I'd like to avoid unnecessary behavioural changes in this monster CL :p
This is using `??` instead of `||`, so it should be the same: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing
Agreed that we should avoid behavioral changes in this CL. We should keep this as-is.
// The wallpaper controller is expected to impose max resolution.Fred ShihI think we should probably undo this change, and just adjust line 341 above to match the other tests in this file.
The previous setup no longer works and is somewhat brittle. It used to work because the forEach photo was an object, so it was passed by reference. Therefore url += '=s512' would mutate the existing object. Now that photos is a string, it is copied in the iteration, so photo += '=s512' no longer mutates the existing photos object.
It's just a bad pattern to hold on to a large object that is mutated throughout the test. It took me a while to figure out why this test broke :\
- Demetrios Papadopoulos
- Fred Shih
- Roman Arora
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. |
Roman Arora (Gerrit)
Roman Arora voted and added 1 comment![]( "Open in Gerrit")
Votes added by Roman Arora
| Code-Review | +1 |
1 comment
Demetrios Papadopoulos@roman, would it be better if I wait until next year to submit this?
Roman AroraPerhaps right before the branch cut, so that anyone who needs to merge after the branch cut can do so more easily?
yes, ideally before the M143 branch cut, I imagine Friday Oct 24th would be a good date, thanks!
Related details
- Demetrios Papadopoulos
- Fred Shih
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fred Shih (Gerrit)
Fred Shih added 1 comment![]( "Open in Gerrit")
unfortunately (or fortunately??) I am going to be busy with a trip and then will be going on vacation soon. Rebasing this CL has been quite time consuming and complicated, so I'll probably leave this until after I come back.
I do want to look more deeply into the personalization page. That page seem to be dropping type information in their code which makes it very difficult to migrate their code correctly. I've spent most of my time on that page and that's the page that breaks after every rebase... Maybe we can do something about the page's mojom defs or impl to allow the compiler to catch more issues.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 1 comment![]( "Open in Gerrit")
unfortunately (or fortunately??) I am going to be busy with a trip and then will be going on vacation soon. Rebasing this CL has been quite time consuming and complicated, so I'll probably leave this until after I come back.
I do want to look more deeply into the personalization page. That page seem to be dropping type information in their code which makes it very difficult to migrate their code correctly. I've spent most of my time on that page and that's the page that breaks after every rebase... Maybe we can do something about the page's mojom defs or impl to allow the compiler to catch more issues.
Any plans to pick up that CL again soon? Asking to evaluate whether we should/could try to push this forward from the WebUI side in Q1. Thanks!
Related details
- Fred Shih
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. |
Fred Shih (Gerrit)
Fred Shih added 1 comment![]( "Open in Gerrit")
Demetrios Papadopoulosunfortunately (or fortunately??) I am going to be busy with a trip and then will be going on vacation soon. Rebasing this CL has been quite time consuming and complicated, so I'll probably leave this until after I come back.
I do want to look more deeply into the personalization page. That page seem to be dropping type information in their code which makes it very difficult to migrate their code correctly. I've spent most of my time on that page and that's the page that breaks after every rebase... Maybe we can do something about the page's mojom defs or impl to allow the compiler to catch more issues.
Any plans to pick up that CL again soon? Asking to evaluate whether we should/could try to push this forward from the WebUI side in Q1. Thanks!
- Demetrios Papadopoulos
- Michael Cui
- Roman Arora
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. |
Fred Shih (Gerrit)
Fred Shih added 1 comment![]( "Open in Gerrit")
alrighty! Ready for review. PTAL again, sorry it's (still) huge.
Related details
- Demetrios Papadopoulos
- Michael Cui
- Roman Arora
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. |
Fred Shih (Gerrit)
Fred Shih voted and added 1 comment![]( "Open in Gerrit")
Votes added by Fred Shih
| Commit-Queue | +1 |
Demetrios Papadopoulosunfortunately (or fortunately??) I am going to be busy with a trip and then will be going on vacation soon. Rebasing this CL has been quite time consuming and complicated, so I'll probably leave this until after I come back.
I do want to look more deeply into the personalization page. That page seem to be dropping type information in their code which makes it very difficult to migrate their code correctly. I've spent most of my time on that page and that's the page that breaks after every rebase... Maybe we can do something about the page's mojom defs or impl to allow the compiler to catch more issues.
Fred ShihAny plans to pick up that CL again soon? Asking to evaluate whether we should/could try to push this forward from the WebUI side in Q1. Thanks!
Yes! I will get a working CL by EoD.
Done
Related details
- Demetrios Papadopoulos
- Michael Cui
- Roman Arora
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Cui (Gerrit)
Michael Cui voted and added 4 comments![]( "Open in Gerrit")
Votes added by Michael Cui
| Code-Review | +1 |
4 comments
// Find pgcl value in request url.nit: Was this comment removal unintentional?
nit: Was this blank line removal unintentional?
The formatting in this file seems to be very weird, can you revert this file and only commit the .url removal?
${this.searchResult_?.items.map((item, index) => html` <cr-url-list-item url="${item.url}" title="${item.title}"nit: This formatting looks incorrect, we should rever it to the previous formatting (except without the extra .url).
Related details
- Demetrios Papadopoulos
- Fred Shih
- Roman Arora
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. |