Re: FW: PWA moving out to...?– Let me know if some functionality abo...

5 views
Skip to first unread message

Dan Murphy

unread,
Aug 7, 2025, 9:55:53 PMAug 7
to Igor Britsky, pwa-dev
Hi Igor,

Sorry for the delay here, I'm going to delve into this more tomorrow.

+pwa-dev to not have this information silo'd.

I'm unsure how ok it will be for having a lot of circular dependencies here until the move... or the api at the end here.... is complete. I worry about the technical hurdles that will have for our team in the meantime, and the number of swe cycles this will cost us to work with. Especially if we need to do the hard cleanup at the end - like removing the app_service dependency, fixing the circular dependency on webapps -> web_applications -> webapps after this refactor.

So I'm not sure I can be OK with 'just moving files over' without introducing a better layering system here in the code. I'm not sure how to describe that better.

Perhaps a better way here is to:
  • Start moving files that have no other dependencies in chrome/. We need to remove our app_service dependency here, perhaps a start is moving those types to new ones in webapps. That I would be happy to approve - moving more and more of our basic types over to webapps/browser
  • I'm also happy to +1 CLs that change the namespace of web_app to webapps everywhere in the code. We need to do that. That will also make your final goal easier to obtain as well.
What do you think about that?

Dan

On Thu, Jul 24, 2025 at 6:57 AM Igor Britsky <igor.b...@lge.com> wrote:

Hello Dan!


Sorry for the long silence.

I`ll try to go back to my idea to avoid usage the //chrome/browser/web_applications in out PWA for the webOS OSE solution.


I collect types and methods, which used directly. Here it is:


//chrome/browser/web_applications/web_app_install_info.h

web_app::WebAppInstallInfo;

web_app::IconsMap;

web_app::DownloadedIconsHttpResults;

//chrome/browser/web_applications/web_app_constants.h

web_app::IconsDownloadedResult;

//chrome/browser/web_applications/web_contents/web_app_data_retriever.h

web_app::WebAppDataRetriever;

//chrome/browser/web_applications/web_app_install_utils.h

base::flat_set<GURL> web_app::GetValidIconUrlsToDownload(const web_app::WebAppInstallInfo& web_app_info);

void web_app::PopulateProductIcons(web_app::WebAppInstallInfo* web_app_info, const web_app::IconsMap* icons_map);

void web_app::PopulateOtherIcons(web_app::WebAppInstallInfo* web_app_info, const web_app::IconsMap& icons_map);

void web_app::UpdateWebAppInfoFromManifest(const blink::mojom::Manifest& manifest, const GURL& manifest_url, web_app::WebAppInstallInfo* web_app_info);


all other stuff, that we try to move to the //components/webapps/ - grows from this points.



But I try to imagine the API you suggested. And I can`t figure out how to make an API in //components/webapps - and not to move out code from //chome.


So, if we create some API in //components, like //components/webapps/browser/web_app_info_and_icon_retriever - we need to move implementation here.

Am I right?


But I`ve just trying to do it in my proposed patchsets https://chromium-review.googlesource.com/c/chromium/src/+/6585513 

And I came across a number of dependecies, of which there weren`t that many.


Can you explain your idea, based on the my dependencies, listed above?



Some links to restore our context:

my last attempt to CR the changes - https://chromium-review.googlesource.com/c/chromium/src/+/6585513/1

explanation doc - https://docs.google.com/document/d/1A1wTqrEO2eHAkXKQFbYeoaFkv7hk6XIzX2JtVJhSkms

bug - https://issues.chromium.org/issues/392317687


---------- Original Message ----------

From : "Dan Murphy (Google Документы)" <comments...@docs.google.com>
To : Igor Britsky Senior Research Engineer(igor.britsky)
Date : 2025/05/27 21:13:53 [GMT+03:00]
Subject : PWA moving out to... – Let me know if some functionality abo...


Пользователь Dan Murphy (dmu...@google.com) упомянул вас в комментарии в следующем документе:

1 комментарий

s there will DEFINITELY be things that need to stay in 'chrome'.

Igor Britsky

• 30 апр. в 07:38 (EST)
Let me know if some functionality above should stay at Chrome.
According our current vision - all of them can be moved out of //chrome

Dan Murphy

• 6 мая в 19:11 (EST)
(Make sure to @me so I get notified when you comment here. Or email me to take a look).

I can tell you likely a lot of those have cross-dependencies with other stuff that you won't want to move out. Hard to know.

Why, for example, do you want "web_app_install_info.h"?  I expect we will change this over time, and we can't be careful to make sure we don't break you. I would recommend probably to stick with the spec-defined types to help with not being broken.

A risk this needs to keep in mind is being broken from us changing stuff.

Dan Murphy

• 27 мая в 13:54 (EST)
@igor.b...@lge.com any update on this? Another example is Chrome Sync - we are heavily integrated with the Chrome Sync system. You expect to move that out of //chrome?

Dan Murphy

• 27 мая в 14:08 (EST)

Новое

To be more specific:

There is no way that this system can simply be copy-pasted between chrome/browser/web_applications to components/webapps due to a lot of deep dependencies on /chrome/ subsystems that cannot be moved out of /chrome/. Thus - there will be some kind of API used between these layers, and that cannot be avoided.

This API (or at least the general layering of responsibilities) needs to be determined first before we can start moving code.

In order to figure out what that API / layer will be, the exact use-cases that the common code need to solve needs to be outlined. I do not see that sufficiently done here yet.

Google LLC, 1600 Amphitheatre Parkway, Mountain View, CA 94043, USA

Вы получили это письмо, так как пользователь Dan Murphy (dmu...@google.com) упомянул вас в этом обсуждении. Если вы не хотите получать файлы от этого пользователя, заблокируйте его на Диске. Вы можете изменить настройки уведомлений от Google. Чтобы принять участие в обсуждении, ответьте на это письмо.

Reply all
Reply to author
Forward
0 new messages